| Message ID | 20250522125521.6465-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit : > Now that video pool creation is handled by a dedicated function, the > logic can be simplified by returning early instead of nesting scopes. Do > so to decrease indentation and improve readability. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++----------------- > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 380f8368af8b..e0eb6e7b4227 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self, > { > g_autoptr(GstQuery) query = NULL; > const gboolean need_pool = true; > - gboolean has_video_meta = false; > GstBufferPool *video_pool = NULL; > > gst_libcamera_extrapolate_info(info, stream_cfg.stride); > > query = gst_query_new_allocation(caps, need_pool); > - if (!gst_pad_peer_query(srcpad, query)) > + if (!gst_pad_peer_query(srcpad, query)) { > GST_DEBUG_OBJECT(self, "Didn't get downstream ALLOCATION hints"); > - else > - has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL); > + return { NULL, 0 }; Ah, so this is your use of a tuple. It wasn't obvious in previous patch that there was this case too. Please document briefly, it will help maintain this in the long run. > + } > > - if (!has_video_meta) { > - GstBufferPool *pool = NULL; > + if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)) > + return { NULL, 0 }; > > - if (gst_query_get_n_allocation_pools(query) > 0) > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > + if (gst_query_get_n_allocation_pools(query) > 0) > + gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL); Once we figure-out a way to tell libcamera the number of buffers (just allocating N requests does not seem to do it for me), the min should also be propagated, that will fix more stalls. For this reason, it might be nice to move this before the find function, so we later have a chance to save and use the min. > > - if (pool) > - video_pool = pool; > - else { > - GstStructure *config; > - guint min_buffers = 3; > - video_pool = gst_video_buffer_pool_new(); > + if (!video_pool) { > + GstStructure *config; > + guint min_buffers = 3; > > - config = gst_buffer_pool_get_config(video_pool); > - gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); > + video_pool = gst_video_buffer_pool_new(); > + config = gst_buffer_pool_get_config(video_pool); > + gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); To be fixed in future update, I missed that min_buffers has been accidentally hard coded. VideoPool is very flexible, but to avoid run-time allocation you should use the information from gst_query_parse_nth_allocation_pool() to configure the pool. One of the "NULL" hides the actual pipeline reported min. Since this is for the copy case, we don't need an inflight buffer and can simply pass over the min/max. Normally we try and use the size returned downstream too, downstream may want some tail padding which VideoPool can accomodate for. > > - GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > + GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > > - gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > - } > + gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > + } > > - GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > + GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > > - if (!gst_buffer_pool_set_active(video_pool, true)) { > - gst_caps_unref(caps); > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > - ("Failed to active buffer pool"), > - ("gst_libcamera_src_negotiate() failed.")); > - return { NULL, -EINVAL }; > - } > + if (!gst_buffer_pool_set_active(video_pool, true)) { > + gst_caps_unref(caps); > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > + ("Failed to active buffer pool"), > + ("gst_libcamera_src_negotiate() failed.")); > + return { NULL, -EINVAL }; There seems to be more issues in that function, the pool configuration does not seem complete and you may fail here trying to activate an unconfigured pool. But this is not related to your changes so: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> If I find anytime, I will provide some patches, yet I'm like you, I don't have a setup to actually test it properly. regards, Nicolas > } > > return { video_pool, 0 };
Hi, > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 2025年5月22日 20:55 > To: libcamera-devel@lists.libcamera.org > Cc: Qi Hou <qi.hou@nxp.com>; Nicolas Dufresne > <nicolas.dufresne@collabora.com> > Subject: [EXT] [PATCH 4/4] gstreamer: Reduce indentation in > gst_libcamera_create_video_pool() > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > Now that video pool creation is handled by a dedicated function, the logic can > be simplified by returning early instead of nesting scopes. Do so to decrease > indentation and improve readability. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++----------------- > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > b/src/gstreamer/gstlibcamerasrc.cpp > index 380f8368af8b..e0eb6e7b4227 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc > *self, { > g_autoptr(GstQuery) query = NULL; > const gboolean need_pool = true; > - gboolean has_video_meta = false; > GstBufferPool *video_pool = NULL; > > gst_libcamera_extrapolate_info(info, stream_cfg.stride); > > query = gst_query_new_allocation(caps, need_pool); > - if (!gst_pad_peer_query(srcpad, query)) > + if (!gst_pad_peer_query(srcpad, query)) { > GST_DEBUG_OBJECT(self, "Didn't get downstream > ALLOCATION hints"); > - else > - has_video_meta = gst_query_find_allocation_meta(query, > GST_VIDEO_META_API_TYPE, NULL); > + return { NULL, 0 }; > + } I 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. Regards, Qi Hou > > - if (!has_video_meta) { > - GstBufferPool *pool = NULL; > + if (gst_query_find_allocation_meta(query, > GST_VIDEO_META_API_TYPE, NULL)) > + return { NULL, 0 }; > > - if (gst_query_get_n_allocation_pools(query) > 0) > - gst_query_parse_nth_allocation_pool(query, 0, > &pool, NULL, NULL, NULL); > + if (gst_query_get_n_allocation_pools(query) > 0) > + gst_query_parse_nth_allocation_pool(query, 0, > + &video_pool, NULL, NULL, NULL); > > - if (pool) > - video_pool = pool; > - else { > - GstStructure *config; > - guint min_buffers = 3; > - video_pool = gst_video_buffer_pool_new(); > + if (!video_pool) { > + GstStructure *config; > + guint min_buffers = 3; > > - config = gst_buffer_pool_get_config(video_pool); > - gst_buffer_pool_config_set_params(config, caps, > info->size, min_buffers, 0); > + video_pool = gst_video_buffer_pool_new(); > + config = gst_buffer_pool_get_config(video_pool); > + gst_buffer_pool_config_set_params(config, caps, > + info->size, min_buffers, 0); > > - GST_DEBUG_OBJECT(self, "Own pool config is %" > GST_PTR_FORMAT, config); > + GST_DEBUG_OBJECT(self, "Own pool config is %" > + GST_PTR_FORMAT, config); > > - > gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > - } > + > gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > + } > > - GST_WARNING_OBJECT(self, "Downstream doesn't support > video meta, need to copy frame."); > + GST_WARNING_OBJECT(self, "Downstream doesn't support video > meta, > + need to copy frame."); > > - if (!gst_buffer_pool_set_active(video_pool, true)) { > - gst_caps_unref(caps); > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > - ("Failed to active buffer > pool"), > - > ("gst_libcamera_src_negotiate() failed.")); > - return { NULL, -EINVAL }; > - } > + if (!gst_buffer_pool_set_active(video_pool, true)) { > + gst_caps_unref(caps); > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > + ("Failed to active buffer pool"), > + ("gst_libcamera_src_negotiate() > failed.")); > + return { NULL, -EINVAL }; > } > > return { video_pool, 0 }; > -- > Regards, > > Laurent Pinchart
On Thu, May 22, 2025 at 01:53:44PM -0400, Nicolas Dufresne wrote: > Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit : > > Now that video pool creation is handled by a dedicated function, the > > logic can be simplified by returning early instead of nesting scopes. Do > > so to decrease indentation and improve readability. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 27 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 380f8368af8b..e0eb6e7b4227 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self, > > { > > g_autoptr(GstQuery) query = NULL; > > const gboolean need_pool = true; > > - gboolean has_video_meta = false; > > GstBufferPool *video_pool = NULL; > > > > gst_libcamera_extrapolate_info(info, stream_cfg.stride); > > > > query = gst_query_new_allocation(caps, need_pool); > > - if (!gst_pad_peer_query(srcpad, query)) > > + if (!gst_pad_peer_query(srcpad, query)) { > > GST_DEBUG_OBJECT(self, "Didn't get downstream ALLOCATION hints"); > > - else > > - has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL); > > + return { NULL, 0 }; > > Ah, so this is your use of a tuple. It wasn't obvious in previous patch that there was this case > too. Please document briefly, it will help maintain this in the long run. Sure, I'll document it. > > + } > > > > - if (!has_video_meta) { > > - GstBufferPool *pool = NULL; > > + if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)) > > + return { NULL, 0 }; > > > > - if (gst_query_get_n_allocation_pools(query) > 0) > > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > > + if (gst_query_get_n_allocation_pools(query) > 0) > > + gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL); > > Once we figure-out a way to tell libcamera the number of buffers (just allocating N requests > does not seem to do it for me), the min should also be propagated, that will fix more stalls. > > For this reason, it might be nice to move this before the find function, so we later have a > chance to save and use the min. I'm fine with that, but I'd like to do so in a separate patch, to avoid introducing functional changes here. I have no idea what those function calls do and if their order is important :-) > > > > - if (pool) > > - video_pool = pool; > > - else { > > - GstStructure *config; > > - guint min_buffers = 3; > > - video_pool = gst_video_buffer_pool_new(); > > + if (!video_pool) { > > + GstStructure *config; > > + guint min_buffers = 3; > > > > - config = gst_buffer_pool_get_config(video_pool); > > - gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); > > + video_pool = gst_video_buffer_pool_new(); > > + config = gst_buffer_pool_get_config(video_pool); > > + gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); > > To be fixed in future update, I missed that min_buffers has been accidentally hard > coded. VideoPool is very flexible, but to avoid run-time allocation you should use the > information from gst_query_parse_nth_allocation_pool() to configure the pool. > > One of the "NULL" hides the actual pipeline reported min. Since this is for the copy case, > we don't need an inflight buffer and can simply pass over the min/max. Normally we try and > use the size returned downstream too, downstream may want some tail padding which VideoPool > can accomodate for. > > > > > - GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > > + GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > > > > - gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > > - } > > + gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > > + } > > > > - GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > > + GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > > > > - if (!gst_buffer_pool_set_active(video_pool, true)) { > > - gst_caps_unref(caps); > > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > - ("Failed to active buffer pool"), > > - ("gst_libcamera_src_negotiate() failed.")); > > - return { NULL, -EINVAL }; > > - } > > + if (!gst_buffer_pool_set_active(video_pool, true)) { > > + gst_caps_unref(caps); > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > + ("Failed to active buffer pool"), > > + ("gst_libcamera_src_negotiate() failed.")); > > + return { NULL, -EINVAL }; > > There seems to be more issues in that function, the pool configuration does not seem > complete and you may fail here trying to activate an unconfigured pool. But this is > not related to your changes so: > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > If I find anytime, I will provide some patches, yet I'm like you, I don't have a setup > to actually test it properly. That would be very nice, as this is really outside of my comfort zone. Maybe Hou could test the patches ? > > } > > > > return { video_pool, 0 };
On Fri, May 23, 2025 at 05:58:36AM +0000, Qi Hou wrote: > On 2025年5月22日 20:55, Laurent Pinchart wrote: > > > > Now that video pool creation is handled by a dedicated function, the logic can > > be simplified by returning early instead of nesting scopes. Do so to decrease > > indentation and improve readability. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 27 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > b/src/gstreamer/gstlibcamerasrc.cpp > > index 380f8368af8b..e0eb6e7b4227 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc > > *self, { > > g_autoptr(GstQuery) query = NULL; > > const gboolean need_pool = true; > > - gboolean has_video_meta = false; > > GstBufferPool *video_pool = NULL; > > > > gst_libcamera_extrapolate_info(info, stream_cfg.stride); > > > > query = gst_query_new_allocation(caps, need_pool); > > - if (!gst_pad_peer_query(srcpad, query)) > > + if (!gst_pad_peer_query(srcpad, query)) { > > GST_DEBUG_OBJECT(self, "Didn't get downstream ALLOCATION hints"); > > - else > > - has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL); > > + return { NULL, 0 }; > > + } > > I 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. You're right, I introduced a change in behaviour by mistake here. I'll fix it. Thank you for noticing. > > - if (!has_video_meta) { > > - GstBufferPool *pool = NULL; > > + if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)) > > + return { NULL, 0 }; > > > > - if (gst_query_get_n_allocation_pools(query) > 0) > > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > > + if (gst_query_get_n_allocation_pools(query) > 0) > > + gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL); > > > > - if (pool) > > - video_pool = pool; > > - else { > > - GstStructure *config; > > - guint min_buffers = 3; > > - video_pool = gst_video_buffer_pool_new(); > > + if (!video_pool) { > > + GstStructure *config; > > + guint min_buffers = 3; > > > > - config = gst_buffer_pool_get_config(video_pool); > > - gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); > > + video_pool = gst_video_buffer_pool_new(); > > + config = gst_buffer_pool_get_config(video_pool); > > + gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); > > > > - GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > > + GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); > > > > - gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > > - } > > + gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); > > + } > > > > - GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > > + GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); > > > > - if (!gst_buffer_pool_set_active(video_pool, true)) { > > - gst_caps_unref(caps); > > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > - ("Failed to active buffer pool"), > > - ("gst_libcamera_src_negotiate() failed.")); > > - return { NULL, -EINVAL }; > > - } > > + if (!gst_buffer_pool_set_active(video_pool, true)) { > > + gst_caps_unref(caps); > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > + ("Failed to active buffer pool"), > > + ("gst_libcamera_src_negotiate() failed.")); > > + return { NULL, -EINVAL }; > > } > > > > return { video_pool, 0 };
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 380f8368af8b..e0eb6e7b4227 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -528,47 +528,43 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self, { g_autoptr(GstQuery) query = NULL; const gboolean need_pool = true; - gboolean has_video_meta = false; GstBufferPool *video_pool = NULL; gst_libcamera_extrapolate_info(info, stream_cfg.stride); query = gst_query_new_allocation(caps, need_pool); - if (!gst_pad_peer_query(srcpad, query)) + if (!gst_pad_peer_query(srcpad, query)) { GST_DEBUG_OBJECT(self, "Didn't get downstream ALLOCATION hints"); - else - has_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL); + return { NULL, 0 }; + } - if (!has_video_meta) { - GstBufferPool *pool = NULL; + if (gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE, NULL)) + return { NULL, 0 }; - if (gst_query_get_n_allocation_pools(query) > 0) - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); + if (gst_query_get_n_allocation_pools(query) > 0) + gst_query_parse_nth_allocation_pool(query, 0, &video_pool, NULL, NULL, NULL); - if (pool) - video_pool = pool; - else { - GstStructure *config; - guint min_buffers = 3; - video_pool = gst_video_buffer_pool_new(); + if (!video_pool) { + GstStructure *config; + guint min_buffers = 3; - config = gst_buffer_pool_get_config(video_pool); - gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); + video_pool = gst_video_buffer_pool_new(); + config = gst_buffer_pool_get_config(video_pool); + gst_buffer_pool_config_set_params(config, caps, info->size, min_buffers, 0); - GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); + GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config); - gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); - } + gst_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config); + } - GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); + GST_WARNING_OBJECT(self, "Downstream doesn't support video meta, need to copy frame."); - if (!gst_buffer_pool_set_active(video_pool, true)) { - gst_caps_unref(caps); - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, - ("Failed to active buffer pool"), - ("gst_libcamera_src_negotiate() failed.")); - return { NULL, -EINVAL }; - } + if (!gst_buffer_pool_set_active(video_pool, true)) { + gst_caps_unref(caps); + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, + ("Failed to active buffer pool"), + ("gst_libcamera_src_negotiate() failed.")); + return { NULL, -EINVAL }; } return { video_pool, 0 };
Now that video pool creation is handled by a dedicated function, the logic can be simplified by returning early instead of nesting scopes. Do so to decrease indentation and improve readability. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 50 ++++++++++++++----------------- 1 file changed, 23 insertions(+), 27 deletions(-)