| Message ID | 20250522125521.6465-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit : > The gst_libcamera_src_negotiate() function uses 5 identation levels, > causing long lines. Move video pool creation to a separate function to > increase readability. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 105 +++++++++++++++++------------- > 1 file changed, 61 insertions(+), 44 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 70bb0606c72c..71f5700d9de7 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -29,6 +29,7 @@ > > #include <atomic> > #include <queue> > +#include <tuple> > #include <vector> > > #include <libcamera/camera.h> > @@ -519,6 +520,61 @@ gst_libcamera_src_open(GstLibcameraSrc *self) > return true; > } > > +static std::tuple<GstBufferPool *, int> Either document the return values, or drop the tuple. Note that I prefer the second since there is no value added in the usage of tuple, since it has only one error code which always imply a nullptr pool pointer. Being a private helper, it can be extended later. > +gst_libcamera_create_video_pool(GstLibcameraSrc *self, > + const StreamConfiguration &stream_cfg, > + GstVideoInfo *info, GstPad *srcpad, > + GstCaps *caps) > +{ > + GstQuery *query = NULL; > + const gboolean need_pool = true; > + gboolean has_video_meta = false; > + GstBufferPool *video_pool = NULL; Oops, NULL started to re-appear, when I wrote the initial version I was asked to turn them all into nullptr instead. It might be a good moment to fix it, of course separate from the refactoring. > + > + gst_libcamera_extrapolate_info(info, stream_cfg.stride); > + > + query = gst_query_new_allocation(caps, need_pool); > + 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); > + > + if (!has_video_meta) { > + GstBufferPool *pool = NULL; > + > + if (gst_query_get_n_allocation_pools(query) > 0) > + gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > + > + if (pool) > + video_pool = pool; > + else { > + GstStructure *config; > + guint min_buffers = 3; > + 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_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."); > + > + 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 }; > + } > + } > + > + gst_query_unref(query); > + return { video_pool, 0 }; > +} > + > /* Must be called with stream_lock held. */ > static bool > gst_libcamera_src_negotiate(GstLibcameraSrc *self) > @@ -601,50 +657,11 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > /* Stride mismatch between camera stride and that calculated by video-info. */ > if (static_cast<unsigned int>(info.stride[0]) != stream_cfg.stride && > GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) { > - GstQuery *query = NULL; > - const gboolean need_pool = true; > - gboolean has_video_meta = false; > - > - gst_libcamera_extrapolate_info(&info, stream_cfg.stride); > - > - query = gst_query_new_allocation(caps, need_pool); > - 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); > - > - if (!has_video_meta) { > - GstBufferPool *pool = NULL; > - > - if (gst_query_get_n_allocation_pools(query) > 0) > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > - > - if (pool) > - video_pool = pool; > - else { > - GstStructure *config; > - guint min_buffers = 3; > - 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_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."); > - > - 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 false; > - } > - } > - gst_query_unref(query); > + std::tie(video_pool, ret) = > + gst_libcamera_create_video_pool(self, stream_cfg, > + &info, srcpad, caps); > + if (ret) > + return false; This is good improvement otherwise. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> regards, Nicolas > } > > GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
On Thu, May 22, 2025 at 01:34:57PM -0400, Nicolas Dufresne wrote: > > Hi, > > Le jeudi 22 mai 2025 à 14:55 +0200, Laurent Pinchart a écrit : > > The gst_libcamera_src_negotiate() function uses 5 identation levels, > > causing long lines. Move video pool creation to a separate function to > > increase readability. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 105 +++++++++++++++++------------- > > 1 file changed, 61 insertions(+), 44 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 70bb0606c72c..71f5700d9de7 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -29,6 +29,7 @@ > > > > #include <atomic> > > #include <queue> > > +#include <tuple> > > #include <vector> > > > > #include <libcamera/camera.h> > > @@ -519,6 +520,61 @@ gst_libcamera_src_open(GstLibcameraSrc *self) > > return true; > > } > > > > +static std::tuple<GstBufferPool *, int> > > Either document the return values, or drop the tuple. Note that I prefer the > second since there is no value added in the usage of tuple, since it has only > one error code which always imply a nullptr pool pointer. Being a private > helper, it can be extended later. As discussed in patch 4/4, the error code is needed. I will document the function, but you'll see from the documentation in v2 that I don't fully understand what it does :-) > > +gst_libcamera_create_video_pool(GstLibcameraSrc *self, > > + const StreamConfiguration &stream_cfg, > > + GstVideoInfo *info, GstPad *srcpad, > > + GstCaps *caps) > > +{ > > + GstQuery *query = NULL; > > + const gboolean need_pool = true; > > + gboolean has_video_meta = false; > > + GstBufferPool *video_pool = NULL; > > Oops, NULL started to re-appear, when I wrote the initial version I was asked to > turn them all into nullptr instead. It might be a good moment to fix it, of course > separate from the refactoring. OK, I'll do so. > > + > > + gst_libcamera_extrapolate_info(info, stream_cfg.stride); > > + > > + query = gst_query_new_allocation(caps, need_pool); > > + 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); > > + > > + if (!has_video_meta) { > > + GstBufferPool *pool = NULL; > > + > > + if (gst_query_get_n_allocation_pools(query) > 0) > > + gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > > + > > + if (pool) > > + video_pool = pool; > > + else { > > + GstStructure *config; > > + guint min_buffers = 3; > > + 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_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."); > > + > > + if (!gst_buffer_pool_set_active(video_pool, true)) { > > + gst_caps_unref(caps); I think this is a but. I'll add a new patch in v2 to fix it. > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > + ("Failed to active buffer pool"), > > + ("gst_libcamera_src_negotiate() failed.")); > > + return { NULL, -EINVAL }; > > + } > > + } > > + > > + gst_query_unref(query); > > + return { video_pool, 0 }; > > +} > > + > > /* Must be called with stream_lock held. */ > > static bool > > gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > @@ -601,50 +657,11 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > /* Stride mismatch between camera stride and that calculated by video-info. */ > > if (static_cast<unsigned int>(info.stride[0]) != stream_cfg.stride && > > GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) { > > - GstQuery *query = NULL; > > - const gboolean need_pool = true; > > - gboolean has_video_meta = false; > > - > > - gst_libcamera_extrapolate_info(&info, stream_cfg.stride); > > - > > - query = gst_query_new_allocation(caps, need_pool); > > - 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); > > - > > - if (!has_video_meta) { > > - GstBufferPool *pool = NULL; > > - > > - if (gst_query_get_n_allocation_pools(query) > 0) > > - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); > > - > > - if (pool) > > - video_pool = pool; > > - else { > > - GstStructure *config; > > - guint min_buffers = 3; > > - 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_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."); > > - > > - 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 false; > > - } > > - } > > - gst_query_unref(query); > > + std::tie(video_pool, ret) = > > + gst_libcamera_create_video_pool(self, stream_cfg, > > + &info, srcpad, caps); > > + if (ret) > > + return false; > > This is good improvement otherwise. > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > } > > > > GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 70bb0606c72c..71f5700d9de7 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -29,6 +29,7 @@ #include <atomic> #include <queue> +#include <tuple> #include <vector> #include <libcamera/camera.h> @@ -519,6 +520,61 @@ gst_libcamera_src_open(GstLibcameraSrc *self) return true; } +static std::tuple<GstBufferPool *, int> +gst_libcamera_create_video_pool(GstLibcameraSrc *self, + const StreamConfiguration &stream_cfg, + GstVideoInfo *info, GstPad *srcpad, + GstCaps *caps) +{ + 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)) + 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); + + if (!has_video_meta) { + GstBufferPool *pool = NULL; + + if (gst_query_get_n_allocation_pools(query) > 0) + gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); + + if (pool) + video_pool = pool; + else { + GstStructure *config; + guint min_buffers = 3; + 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_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."); + + 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 }; + } + } + + gst_query_unref(query); + return { video_pool, 0 }; +} + /* Must be called with stream_lock held. */ static bool gst_libcamera_src_negotiate(GstLibcameraSrc *self) @@ -601,50 +657,11 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) /* Stride mismatch between camera stride and that calculated by video-info. */ if (static_cast<unsigned int>(info.stride[0]) != stream_cfg.stride && GST_VIDEO_INFO_FORMAT(&info) != GST_VIDEO_FORMAT_ENCODED) { - GstQuery *query = NULL; - const gboolean need_pool = true; - gboolean has_video_meta = false; - - gst_libcamera_extrapolate_info(&info, stream_cfg.stride); - - query = gst_query_new_allocation(caps, need_pool); - 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); - - if (!has_video_meta) { - GstBufferPool *pool = NULL; - - if (gst_query_get_n_allocation_pools(query) > 0) - gst_query_parse_nth_allocation_pool(query, 0, &pool, NULL, NULL, NULL); - - if (pool) - video_pool = pool; - else { - GstStructure *config; - guint min_buffers = 3; - 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_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."); - - 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 false; - } - } - gst_query_unref(query); + std::tie(video_pool, ret) = + gst_libcamera_create_video_pool(self, stream_cfg, + &info, srcpad, caps); + if (ret) + return false; } GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
The gst_libcamera_src_negotiate() function uses 5 identation levels, causing long lines. Move video pool creation to a separate function to increase readability. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 105 +++++++++++++++++------------- 1 file changed, 61 insertions(+), 44 deletions(-)