[{"id":34415,"web_url":"https://patchwork.libcamera.org/comment/34415/","msgid":"<4614c51a4df804cc4694543164f12af14ec5e856.camel@collabora.com>","date":"2025-06-04T17:08:16","subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le mercredi 04 juin 2025 à 16:07 +0300, Laurent Pinchart a écrit :\n> The gst_libcamera_src_negotiate() function uses 5 identation levels,\n> causing long lines. Move video pool creation to a separate function to\n> increase readability.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n> \n> - Document the gst_libcamera_create_video_pool() function\n> - Keep the gst_libcamera_extrapolate_info() call out of the new function\n> - Reorder function arguments\n> ---\n>  src/gstreamer/gstlibcamerasrc.cpp | 114 +++++++++++++++++++-----------\n>  1 file changed, 72 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 6ede43b06a8a..79b94c7ee7c2 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -29,6 +29,7 @@\n> \n>  #include \n>  #include \n> +#include \n>  #include \n> \n>  #include \n> @@ -521,6 +522,72 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n>  \treturn true;\n>  }\n> \n> +/**\n> + * \\brief Create a video pool for a pad\n> + * \\param[in] self The libcamerasrc instance\n> + * \\param[in] srcpad The pad\n> + * \\param[in] caps The pad caps\n> + * \\param[in] info The video info for the pad\n> + *\n> + * This function creates and returns a video buffers pool for the given pad if\n\nUsually no s to buffer when saying \"buffer pool\". Its like \"a noodle soup\" versus\n\"a bowl of noodles\".\n\n> + * needed to accommodate stride mismatch. If the peer element supports the meta\n> + * API, the stride will be negotiated, and no pool if needed.\n\nif needed -> is needed ? \n\n> + *\n> + * \\return A tuple containing the video buffers pool pointer and an error code.\n> + * When no error occurs, the pool can be null if the peer element supports\n> + * stride negotiation.\n\nSounds great.\n\nReviewed-by: Nicolas Dufresne \n\n> + */\n> +static std::tuple\n> +gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,\n> +\t\t\t\tGstCaps *caps, const GstVideoInfo *info)\n> +{\n> +\tGstQuery *query = NULL;\n> +\tconst gboolean need_pool = true;\n> +\tgboolean has_video_meta = false;\n> +\tGstBufferPool *video_pool = NULL;\n> +\n> +\tquery = gst_query_new_allocation(caps, need_pool);\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> +\n> +\tif (!has_video_meta) {\n> +\t\tGstBufferPool *pool = NULL;\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> +\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> +\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> +\n> +\t\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> +\n> +\t\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> +\t}\n> +\n> +\tgst_query_unref(query);\n> +\treturn { video_pool, 0 };\n> +}\n> +\n>  /* Must be called with stream_lock held. */\n>  static bool\n>  gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> @@ -603,50 +670,13 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n>  \t\t/* Stride mismatch between camera stride and that calculated by video-info. */\n>  \t\tif (static_cast(info.stride[0]) != stream_cfg.stride &&\n>  \t\t    GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) {\n> -\t\t\tGstQuery *query = NULL;\n> -\t\t\tconst gboolean need_pool = true;\n> -\t\t\tgboolean has_video_meta = false;\n> -\n>  \t\t\tgst_libcamera_extrapolate_info(&info, stream_cfg.stride);\n> \n> -\t\t\tquery = gst_query_new_allocation(caps, need_pool);\n> -\t\t\tif (!gst_pad_peer_query(srcpad, query))\n> -\t\t\t\tGST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> -\t\t\telse\n> -\t\t\t\thas_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE,\n> NULL);\n> -\n> -\t\t\tif (!has_video_meta) {\n> -\t\t\t\tGstBufferPool *pool = NULL;\n> -\n> -\t\t\t\tif (gst_query_get_n_allocation_pools(query) > 0)\n> -\t\t\t\t\tgst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> -\n> -\t\t\t\tif (pool)\n> -\t\t\t\t\tvideo_pool = pool;\n> -\t\t\t\telse {\n> -\t\t\t\t\tGstStructure *config;\n> -\t\t\t\t\tguint min_buffers = 3;\n> -\t\t\t\t\tvideo_pool = gst_video_buffer_pool_new();\n> -\n> -\t\t\t\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> -\t\t\t\t\tgst_buffer_pool_config_set_params(config, caps, info.size, min_buffers, 0);\n> -\n> -\t\t\t\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> -\n> -\t\t\t\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> -\t\t\t\t}\n> -\n> -\t\t\t\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy\n> frame.\");\n> -\n> -\t\t\t\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> -\t\t\t\t\tgst_caps_unref(caps);\n> -\t\t\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> -\t\t\t\t\t\t\t  (\"Failed to active buffer pool\"),\n> -\t\t\t\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> -\t\t\t\t\treturn false;\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t\tgst_query_unref(query);\n> +\t\t\tstd::tie(video_pool, ret) =\n> +\t\t\t\tgst_libcamera_create_video_pool(self, srcpad,\n> +\t\t\t\t\t\t\t\tcaps, &info);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn false;\n>  \t\t}\n> \n>  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\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 AE396C31E9\n\tfor ;\n\tWed, 4 Jun 2025 17:08:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFD4E68DA8;\n\tWed, 4 Jun 2025 19:08:21 +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 BB6FF68DA8\n\tfor ;\n\tWed, 4 Jun 2025 19:08:19 +0200 (CEST)","from [IPv6:2606:6d00:10:5285::5ac] (unknown\n\t[IPv6:2606:6d00:10:5285::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 77AE717E0FE7;\n\tWed, 4 Jun 2025 19:08:18 +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=\"OXAxc0dX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1749056899;\n\tbh=Up0RUpKMs1N0Dzp7mYX7Z6xuzJs3lTTLxaxi3d88zCo=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=OXAxc0dX2OjkecrahDUmZ5FX+WxB6pcKncmZBBEbdVVEzCdCzq/4P29/f73b4NIL8\n\tbQ9BVYISp3/bFOYtQX958NCs0Zfzy/Xfve2sPBLTdGs/4mc1UP8Ugp8ekgf8+bpLWh\n\tPrmIFcwxhSQq1MURgY1aUWLjYR+9eWq76WYC7H1zSACBKURVadExKkHXNNrr1+FWW7\n\tuqOJmU0gw5U68vEUvoX+Y+4jRa0b/cpUWxz1F/BQCXy0SeaF8kf/zb428Dt2l5LIdh\n\todX4HwTCH2PUU0AHUJdG5GZ7WjmM1Lx8oxLJQ+OwRjfcv/jZINNsoaLBRkTFsVfj70\n\tvpotpRBtIvjeQ==","Message-ID":"<4614c51a4df804cc4694543164f12af14ec5e856.camel@collabora.com>","Subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","From":"Nicolas Dufresne ","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Cc":"Hou Qi ","Date":"Wed, 04 Jun 2025 13:08:16 -0400","In-Reply-To":"<20250604130741.9228-3-laurent.pinchart@ideasonboard.com>","References":"<20250604130741.9228-1-laurent.pinchart@ideasonboard.com>\n\t<20250604130741.9228-3-laurent.pinchart@ideasonboard.com>","Organization":"Collabora Canada","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.56.2 (3.56.2-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":34425,"web_url":"https://patchwork.libcamera.org/comment/34425/","msgid":"<20250605085005.GA20387@pendragon.ideasonboard.com>","date":"2025-06-05T08:50:05","subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 04, 2025 at 01:08:16PM -0400, Nicolas Dufresne wrote:\n> Le mercredi 04 juin 2025 à 16:07 +0300, Laurent Pinchart a écrit :\n> > The gst_libcamera_src_negotiate() function uses 5 identation levels,\n> > causing long lines. Move video pool creation to a separate function to\n> > increase readability.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> > \n> > - Document the gst_libcamera_create_video_pool() function\n> > - Keep the gst_libcamera_extrapolate_info() call out of the new function\n> > - Reorder function arguments\n> > ---\n> >  src/gstreamer/gstlibcamerasrc.cpp | 114 +++++++++++++++++++-----------\n> >  1 file changed, 72 insertions(+), 42 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 6ede43b06a8a..79b94c7ee7c2 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -29,6 +29,7 @@\n> > \n> >  #include \n> >  #include \n> > +#include \n> >  #include \n> > \n> >  #include \n> > @@ -521,6 +522,72 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> >  \treturn true;\n> >  }\n> > \n> > +/**\n> > + * \\brief Create a video pool for a pad\n> > + * \\param[in] self The libcamerasrc instance\n> > + * \\param[in] srcpad The pad\n> > + * \\param[in] caps The pad caps\n> > + * \\param[in] info The video info for the pad\n> > + *\n> > + * This function creates and returns a video buffers pool for the given pad if\n> \n> Usually no s to buffer when saying \"buffer pool\". Its like \"a noodle soup\" versus\n> \"a bowl of noodles\".\n> \n> > + * needed to accommodate stride mismatch. If the peer element supports the meta\n> > + * API, the stride will be negotiated, and no pool if needed.\n> \n> if needed -> is needed ? \n\nOops. I've updated the comment to\n\n * This function creates and returns a video buffer pool for the given pad if\n * needed to accommodate stride mismatch. If the peer element supports stride\n * negotiation through the meta API, no pool is needed and the function will\n * return a null pool.\n *\n * \\return A tuple containing the video buffers pool pointer and an error code\n\n> > + *\n> > + * \\return A tuple containing the video buffers pool pointer and an error code.\n> > + * When no error occurs, the pool can be null if the peer element supports\n> > + * stride negotiation.\n> \n> Sounds great.\n> \n> Reviewed-by: Nicolas Dufresne \n> \n> > + */\n> > +static std::tuple\n> > +gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,\n> > +\t\t\t\tGstCaps *caps, const GstVideoInfo *info)\n> > +{\n> > +\tGstQuery *query = NULL;\n> > +\tconst gboolean need_pool = true;\n> > +\tgboolean has_video_meta = false;\n> > +\tGstBufferPool *video_pool = NULL;\n> > +\n> > +\tquery = gst_query_new_allocation(caps, need_pool);\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> > +\n> > +\tif (!has_video_meta) {\n> > +\t\tGstBufferPool *pool = NULL;\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> > +\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> > +\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> > +\n> > +\t\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> > +\n> > +\t\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> > +\t}\n> > +\n> > +\tgst_query_unref(query);\n> > +\treturn { video_pool, 0 };\n> > +}\n> > +\n> >  /* Must be called with stream_lock held. */\n> >  static bool\n> >  gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > @@ -603,50 +670,13 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> >  \t\t/* Stride mismatch between camera stride and that calculated by video-info. */\n> >  \t\tif (static_cast(info.stride[0]) != stream_cfg.stride &&\n> >  \t\t    GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) {\n> > -\t\t\tGstQuery *query = NULL;\n> > -\t\t\tconst gboolean need_pool = true;\n> > -\t\t\tgboolean has_video_meta = false;\n> > -\n> >  \t\t\tgst_libcamera_extrapolate_info(&info, stream_cfg.stride);\n> > \n> > -\t\t\tquery = gst_query_new_allocation(caps, need_pool);\n> > -\t\t\tif (!gst_pad_peer_query(srcpad, query))\n> > -\t\t\t\tGST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> > -\t\t\telse\n> > -\t\t\t\thas_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL);\n> > -\n> > -\t\t\tif (!has_video_meta) {\n> > -\t\t\t\tGstBufferPool *pool = NULL;\n> > -\n> > -\t\t\t\tif (gst_query_get_n_allocation_pools(query) > 0)\n> > -\t\t\t\t\tgst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> > -\n> > -\t\t\t\tif (pool)\n> > -\t\t\t\t\tvideo_pool = pool;\n> > -\t\t\t\telse {\n> > -\t\t\t\t\tGstStructure *config;\n> > -\t\t\t\t\tguint min_buffers = 3;\n> > -\t\t\t\t\tvideo_pool = gst_video_buffer_pool_new();\n> > -\n> > -\t\t\t\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> > -\t\t\t\t\tgst_buffer_pool_config_set_params(config, caps, info.size, min_buffers, 0);\n> > -\n> > -\t\t\t\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> > -\n> > -\t\t\t\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > -\t\t\t\t}\n> > -\n> > -\t\t\t\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy frame.\");\n> > -\n> > -\t\t\t\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> > -\t\t\t\t\tgst_caps_unref(caps);\n> > -\t\t\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > -\t\t\t\t\t\t\t  (\"Failed to active buffer pool\"),\n> > -\t\t\t\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> > -\t\t\t\t\treturn false;\n> > -\t\t\t\t}\n> > -\t\t\t}\n> > -\t\t\tgst_query_unref(query);\n> > +\t\t\tstd::tie(video_pool, ret) =\n> > +\t\t\t\tgst_libcamera_create_video_pool(self, srcpad,\n> > +\t\t\t\t\t\t\t\tcaps, &info);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn false;\n> >  \t\t}\n> > \n> >  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,","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 E4785C31E9\n\tfor ;\n\tThu, 5 Jun 2025 08:50:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 609EC68DB2;\n\tThu, 5 Jun 2025 10:50:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F3B668DB2\n\tfor ;\n\tThu, 5 Jun 2025 10:50:16 +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 F3245593;\n\tThu, 5 Jun 2025 10:50:11 +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=\"u8OObr6w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749113412;\n\tbh=TdGZargbCCwkjc+eiIp+P40jb23ZSV8VGCyul6X4rdk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u8OObr6wWykp11xG5OWnGyiiszu8u/6q+pAiVNxucJIZ8HZDsQcTK9TsSH2ymAUl+\n\thq+4qblI5k22K+MxBPWEbf28OwMIC9s3XomzWJxnGEKzdP4CGthqw5OCbOuk0lcBq6\n\t78KvoOd8WweNeAl9HiZVmXFkysdhzNYPCRP5YqT0=","Date":"Thu, 5 Jun 2025 11:50:05 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org, Hou Qi ","Subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","Message-ID":"<20250605085005.GA20387@pendragon.ideasonboard.com>","References":"<20250604130741.9228-1-laurent.pinchart@ideasonboard.com>\n\t<20250604130741.9228-3-laurent.pinchart@ideasonboard.com>\n\t<4614c51a4df804cc4694543164f12af14ec5e856.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4614c51a4df804cc4694543164f12af14ec5e856.camel@collabora.com>","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":34431,"web_url":"https://patchwork.libcamera.org/comment/34431/","msgid":"<6a0ab557bcbad0ac0b118d5fc831d643f35ef759.camel@collabora.com>","date":"2025-06-05T12:49:55","subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le jeudi 05 juin 2025 à 11:50 +0300, Laurent Pinchart a écrit :\n> On Wed, Jun 04, 2025 at 01:08:16PM -0400, Nicolas Dufresne wrote:\n> > Le mercredi 04 juin 2025 à 16:07 +0300, Laurent Pinchart a écrit :\n> > > The gst_libcamera_src_negotiate() function uses 5 identation levels,\n> > > causing long lines. Move video pool creation to a separate function to\n> > > increase readability.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Changes since v1:\n> > > \n> > > - Document the gst_libcamera_create_video_pool() function\n> > > - Keep the gst_libcamera_extrapolate_info() call out of the new function\n> > > - Reorder function arguments\n> > > ---\n> > >  src/gstreamer/gstlibcamerasrc.cpp | 114 +++++++++++++++++++-----------\n> > >  1 file changed, 72 insertions(+), 42 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 6ede43b06a8a..79b94c7ee7c2 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -29,6 +29,7 @@\n> > > \n> > >  #include \n> > >  #include \n> > > +#include \n> > >  #include \n> > > \n> > >  #include \n> > > @@ -521,6 +522,72 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> > >  \treturn true;\n> > >  }\n> > > \n> > > +/**\n> > > + * \\brief Create a video pool for a pad\n> > > + * \\param[in] self The libcamerasrc instance\n> > > + * \\param[in] srcpad The pad\n> > > + * \\param[in] caps The pad caps\n> > > + * \\param[in] info The video info for the pad\n> > > + *\n> > > + * This function creates and returns a video buffers pool for the given pad if\n> > \n> > Usually no s to buffer when saying \"buffer pool\". Its like \"a noodle soup\" versus\n> > \"a bowl of noodles\".\n> > \n> > > + * needed to accommodate stride mismatch. If the peer element supports the meta\n> > > + * API, the stride will be negotiated, and no pool if needed.\n> > \n> > if needed -> is needed ? \n> \n> Oops. I've updated the comment to\n> \n>  * This function creates and returns a video buffer pool for the given pad if\n>  * needed to accommodate stride mismatch. If the peer element supports stride\n>  * negotiation through the meta API, no pool is needed and the function will\n>  * return a null pool.\n>  *\n>  * \\return A tuple containing the video buffers pool pointer and an error code\n\nAck, thanks.\n\n> \n> > > + *\n> > > + * \\return A tuple containing the video buffers pool pointer and an error code.\n> > > + * When no error occurs, the pool can be null if the peer element supports\n> > > + * stride negotiation.\n> > \n> > Sounds great.\n> > \n> > Reviewed-by: Nicolas Dufresne \n> > \n> > > + */\n> > > +static std::tuple\n> > > +gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,\n> > > +\t\t\t\tGstCaps *caps, const GstVideoInfo *info)\n> > > +{\n> > > +\tGstQuery *query = NULL;\n> > > +\tconst gboolean need_pool = true;\n> > > +\tgboolean has_video_meta = false;\n> > > +\tGstBufferPool *video_pool = NULL;\n> > > +\n> > > +\tquery = gst_query_new_allocation(caps, need_pool);\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> > > +\n> > > +\tif (!has_video_meta) {\n> > > +\t\tGstBufferPool *pool = NULL;\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> > > +\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> > > +\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> > > +\n> > > +\t\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> > > +\n> > > +\t\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> > > +\t}\n> > > +\n> > > +\tgst_query_unref(query);\n> > > +\treturn { video_pool, 0 };\n> > > +}\n> > > +\n> > >  /* Must be called with stream_lock held. */\n> > >  static bool\n> > >  gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > @@ -603,50 +670,13 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > >  \t\t/* Stride mismatch between camera stride and that calculated by video-info. */\n> > >  \t\tif (static_cast(info.stride[0]) != stream_cfg.stride &&\n> > >  \t\t    GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) {\n> > > -\t\t\tGstQuery *query = NULL;\n> > > -\t\t\tconst gboolean need_pool = true;\n> > > -\t\t\tgboolean has_video_meta = false;\n> > > -\n> > >  \t\t\tgst_libcamera_extrapolate_info(&info, stream_cfg.stride);\n> > > \n> > > -\t\t\tquery = gst_query_new_allocation(caps, need_pool);\n> > > -\t\t\tif (!gst_pad_peer_query(srcpad, query))\n> > > -\t\t\t\tGST_DEBUG_OBJECT(self, \"Didn't get downstream ALLOCATION hints\");\n> > > -\t\t\telse\n> > > -\t\t\t\thas_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE,\n> > > NULL);\n> > > -\n> > > -\t\t\tif (!has_video_meta) {\n> > > -\t\t\t\tGstBufferPool *pool = NULL;\n> > > -\n> > > -\t\t\t\tif (gst_query_get_n_allocation_pools(query) > 0)\n> > > -\t\t\t\t\tgst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL);\n> > > -\n> > > -\t\t\t\tif (pool)\n> > > -\t\t\t\t\tvideo_pool = pool;\n> > > -\t\t\t\telse {\n> > > -\t\t\t\t\tGstStructure *config;\n> > > -\t\t\t\t\tguint min_buffers = 3;\n> > > -\t\t\t\t\tvideo_pool = gst_video_buffer_pool_new();\n> > > -\n> > > -\t\t\t\t\tconfig = gst_buffer_pool_get_config(video_pool);\n> > > -\t\t\t\t\tgst_buffer_pool_config_set_params(config, caps, info.size, min_buffers,\n> > > 0);\n> > > -\n> > > -\t\t\t\t\tGST_DEBUG_OBJECT(self, \"Own pool config is %\" GST_PTR_FORMAT, config);\n> > > -\n> > > -\t\t\t\t\tgst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);\n> > > -\t\t\t\t}\n> > > -\n> > > -\t\t\t\tGST_WARNING_OBJECT(self, \"Downstream doesn't support video meta, need to copy\n> > > frame.\");\n> > > -\n> > > -\t\t\t\tif (!gst_buffer_pool_set_active(video_pool, true)) {\n> > > -\t\t\t\t\tgst_caps_unref(caps);\n> > > -\t\t\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > -\t\t\t\t\t\t\t  (\"Failed to active buffer pool\"),\n> > > -\t\t\t\t\t\t\t  (\"gst_libcamera_src_negotiate() failed.\"));\n> > > -\t\t\t\t\treturn false;\n> > > -\t\t\t\t}\n> > > -\t\t\t}\n> > > -\t\t\tgst_query_unref(query);\n> > > +\t\t\tstd::tie(video_pool, ret) =\n> > > +\t\t\t\tgst_libcamera_create_video_pool(self, srcpad,\n> > > +\t\t\t\t\t\t\t\tcaps, &info);\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn false;\n> > >  \t\t}\n> > > \n> > >  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,","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 569EAC3237\n\tfor ;\n\tThu, 5 Jun 2025 12:50:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC77D68DBD;\n\tThu, 5 Jun 2025 14:49:59 +0200 (CEST)","from bali.collaboradmins.com (bali.collaboradmins.com\n\t[148.251.105.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E63CD68D96\n\tfor ;\n\tThu, 5 Jun 2025 14:49:57 +0200 (CEST)","from [IPv6:2606:6d00:10:5285::5ac] (unknown\n\t[IPv6:2606:6d00:10:5285::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\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bali.collaboradmins.com (Postfix) with ESMTPSA id C85CA17E0509;\n\tThu, 5 Jun 2025 14:49:56 +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=\"KcsEbc8q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1749127797;\n\tbh=HBYIl6+duWPcL78Cgl2DTjr+ZxWHZ3F1/AJBderi1B0=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=KcsEbc8q+Hk287ji48T8dsvVHodA2hoAojWVDm2I9fFJ+oHJmBbH5xXnyxbODvW6j\n\tSpmgV/OIlQlzMhPz7J2Pz1Euwkmyg+dgNpPkZTdrNxu8YX4T68JotUVwj+GoZTwbOi\n\twId1RHS3mK/eH68QjdwX0sORvpjq/5cII89HafK1uUmvZifD2nnPzXJXAPFWPYqHTl\n\t3yk3f2Jbq0n9ccreLxFSMJ1wyEkidhf4Jklv60hCWtJ0ZK/dRoVciQHc330cN4J6XL\n\tzaP5my3bv6f868Bww4afJegjAMphElZp3gthgrsnzNwDwwQAUfmKr1pOoJTHVmi/br\n\tu80sM2cFWqPDg==","Message-ID":"<6a0ab557bcbad0ac0b118d5fc831d643f35ef759.camel@collabora.com>","Subject":"Re: [PATCH v2 2/7] gstreamer: Factor out video pool creation","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Hou Qi ","Date":"Thu, 05 Jun 2025 08:49:55 -0400","In-Reply-To":"<20250605085005.GA20387@pendragon.ideasonboard.com>","References":"<20250604130741.9228-1-laurent.pinchart@ideasonboard.com>\n\t<20250604130741.9228-3-laurent.pinchart@ideasonboard.com>\n\t<4614c51a4df804cc4694543164f12af14ec5e856.camel@collabora.com>\n\t<20250605085005.GA20387@pendragon.ideasonboard.com>","Organization":"Collabora Canada","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.56.2 (3.56.2-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\" "}}]