[PATCH v6] gstreamer: Add GstVideoMeta support
Nicolas Dufresne
nicolas at ndufresne.ca
Mon May 12 23:05:36 CEST 2025
Hi,
Le mercredi 04 décembre 2024 à 17:55 +0900, Hou Qi a écrit :
> GStreamer video-info calculated stride and offset may differ from
> those used by the camera.
>
> For stride and offset mismatch, this patch adds video meta to buffer
> if downstream supports VideoMeta through allocation query. Otherwise,
> create a internal VideoPool using the caps, and copy video frame to
> this system memory.
>
> Signed-off-by: Hou Qi <qi.hou at nxp.com>
> ---
> src/gstreamer/gstlibcamera-utils.cpp | 33 +++++++
> src/gstreamer/gstlibcamera-utils.h | 4 +
> src/gstreamer/gstlibcamerapad.cpp | 31 +++++++
> src/gstreamer/gstlibcamerapad.h | 8 ++
> src/gstreamer/gstlibcamerapool.cpp | 18 +++-
> src/gstreamer/gstlibcamerapool.h | 3 +-
> src/gstreamer/gstlibcamerasrc.cpp | 133 ++++++++++++++++++++++++++-
> 7 files changed, 226 insertions(+), 4 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 732987ef..09af9204 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -591,6 +591,39 @@ gst_task_resume(GstTask *task)
> }
> #endif
>
> +#if !GST_CHECK_VERSION(1, 22, 0)
> +/*
> + * Copyright (C) <1999> Erik Walthinsen <omega at cse.ogi.edu>
> + * Library <2002> Ronald Bultje <rbultje at ronald.bitfreak.net>
> + * Copyright (C) <2007> David A. Schleef <ds at schleef.org>
> + */
> +/* This function has been imported directly from the gstreamer project to
> + * support backwards compatibility and should be removed when the older
> + * version is no longer supported. */
> +gint gst_video_format_info_extrapolate_stride(const GstVideoFormatInfo *finfo, gint plane, gint stride)
> +{
> + gint estride;
> + gint comp[GST_VIDEO_MAX_COMPONENTS];
> + gint i;
> +
> + /* there is nothing to extrapolate on first plane */
> + if (plane == 0)
> + return stride;
> +
> + gst_video_format_info_component(finfo, plane, comp);
> +
> + /* For now, all planar formats have a single component on first plane, but
> + * if there was a planar format with more, we'd have to make a ratio of the
> + * number of component on the first plane against the number of component on
> + * the current plane. */
> + estride = 0;
> + for (i = 0; i < GST_VIDEO_MAX_COMPONENTS && comp[i] >= 0; i++)
> + estride += GST_VIDEO_FORMAT_INFO_SCALE_WIDTH(finfo, comp[i], stride);
> +
> + return estride;
> +}
> +#endif
> +
> G_LOCK_DEFINE_STATIC(cm_singleton_lock);
> static std::weak_ptr<CameraManager> cm_singleton_ptr;
>
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index cab1c814..81149280 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -35,6 +35,10 @@ static inline void gst_clear_event(GstEvent **event_ptr)
> #if !GST_CHECK_VERSION(1, 17, 1)
> gboolean gst_task_resume(GstTask *task);
> #endif
> +
> +#if !GST_CHECK_VERSION(1, 22, 0)
> +gint gst_video_format_info_extrapolate_stride(const GstVideoFormatInfo *finfo, gint plane, gint stride);
> +#endif
> std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret);
nit: I'd like a blank line after endif
>
> /**
> diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> index 7b22aebe..7fd11a8f 100644
> --- a/src/gstreamer/gstlibcamerapad.cpp
> +++ b/src/gstreamer/gstlibcamerapad.cpp
> @@ -18,6 +18,8 @@ struct _GstLibcameraPad {
> GstPad parent;
> StreamRole role;
> GstLibcameraPool *pool;
> + GstBufferPool *video_pool;
> + GstVideoInfo info;
> GstClockTime latency;
> };
>
> @@ -153,6 +155,35 @@ gst_libcamera_pad_set_pool(GstPad *pad, GstLibcameraPool *pool)
> self->pool = pool;
> }
>
> +GstBufferPool *
> +gst_libcamera_pad_get_video_pool(GstPad *pad)
> +{
> + auto *self = GST_LIBCAMERA_PAD(pad);
> + return self->video_pool;
> +}
> +
> +void gst_libcamera_pad_set_video_pool(GstPad *pad, GstBufferPool *video_pool)
> +{
> + auto *self = GST_LIBCAMERA_PAD(pad);
> +
> + if (self->video_pool)
> + g_object_unref(self->video_pool);
> + self->video_pool = video_pool;
> +}
> +
> +GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad)
> +{
> + auto *self = GST_LIBCAMERA_PAD(pad);
> + return self->info;
> +}
> +
> +void gst_libcamera_pad_set_video_info(GstPad *pad, GstVideoInfo info)
VideoInfo, a large structure, is being copied twice, pass a const
reference so you only copy it once.
> +{
> + auto *self = GST_LIBCAMERA_PAD(pad);
> +
> + self->info = info;
> +}
> +
> Stream *
> gst_libcamera_pad_get_stream(GstPad *pad)
> {
> diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h
> index 630c168a..ed0beb1f 100644
> --- a/src/gstreamer/gstlibcamerapad.h
> +++ b/src/gstreamer/gstlibcamerapad.h
> @@ -23,6 +23,14 @@ GstLibcameraPool *gst_libcamera_pad_get_pool(GstPad *pad);
>
> void gst_libcamera_pad_set_pool(GstPad *pad, GstLibcameraPool *pool);
>
> +GstBufferPool *gst_libcamera_pad_get_video_pool(GstPad *pad);
> +
> +void gst_libcamera_pad_set_video_pool(GstPad *pad, GstBufferPool *video_pool);
> +
> +GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad);
> +
> +void gst_libcamera_pad_set_video_info(GstPad *pad, GstVideoInfo info);
> +
> libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad);
>
> void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency);
> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> index 9cd7eccb..471f71da 100644
> --- a/src/gstreamer/gstlibcamerapool.cpp
> +++ b/src/gstreamer/gstlibcamerapool.cpp
> @@ -14,6 +14,9 @@
>
> #include "gstlibcamera-utils.h"
>
> +#include <gst/gstmeta.h>
nit: gst/gst.h is sufficient and already included. n GStreamer we usually do
one header per library. Unlike C++ it has marginal effect on compilation time.
> +
> +
> using namespace libcamera;
>
> enum {
> @@ -134,8 +137,20 @@ gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> G_TYPE_NONE, 0);
> }
>
> +static void
> +gst_libcamera_buffer_add_video_meta(GstBuffer *buffer, GstVideoInfo *info)
> +{
> + GstVideoMeta *vmeta;
> + vmeta = gst_buffer_add_video_meta_full(buffer, GST_VIDEO_FRAME_FLAG_NONE,
> + GST_VIDEO_INFO_FORMAT(info), GST_VIDEO_INFO_WIDTH(info),
> + GST_VIDEO_INFO_HEIGHT(info), GST_VIDEO_INFO_N_PLANES(info),
> + info->offset, info->stride);
> + GST_META_FLAGS(vmeta) = (GstMetaFlags)(GST_META_FLAGS(vmeta) | GST_META_FLAG_POOLED);
> +}
> +
> GstLibcameraPool *
> -gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream,
> + GstVideoInfo *info)
> {
> auto *pool = GST_LIBCAMERA_POOL(g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr));
>
> @@ -145,6 +160,7 @@ gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> for (gsize i = 0; i < pool_size; i++) {
> GstBuffer *buffer = gst_buffer_new();
> + gst_libcamera_buffer_add_video_meta(buffer, info);
> pool->queue->push_back(buffer);
> }
>
> diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> index 2a7a9c77..02ee4dd4 100644
> --- a/src/gstreamer/gstlibcamerapool.h
> +++ b/src/gstreamer/gstlibcamerapool.h
> @@ -14,6 +14,7 @@
> #include "gstlibcameraallocator.h"
>
> #include <gst/gst.h>
> +#include <gst/video/video.h>
>
> #include <libcamera/stream.h>
>
> @@ -21,7 +22,7 @@
> G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_LIBCAMERA, POOL, GstBufferPool)
>
> GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> - libcamera::Stream *stream);
> + libcamera::Stream *stream, GstVideoInfo *info);
>
> libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self);
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 8efa25f4..82b06eb4 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -268,6 +268,52 @@ GstLibcameraSrcState::requestCompleted(Request *request)
> gst_task_resume(src_->task);
> }
>
> +static void
> +gst_libcamera_extrapolate_info(GstVideoInfo *info, guint32 stride)
> +{
> + guint i, estride;
> + gsize offset = 0;
> +
> + /* this should be updated if tiled formats get added in the future. */
> + for (i = 0; i < GST_VIDEO_INFO_N_PLANES(info); i++) {
> + estride = gst_video_format_info_extrapolate_stride(info->finfo, i, stride);
> + info->stride[i] = estride;
> + info->offset[i] = offset;
> + offset += estride * GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT(info->finfo, i,
> + GST_VIDEO_INFO_HEIGHT(info));
> + }
> +}
> +
> +static GstFlowReturn
> +gst_libcamera_video_frame_copy(GstBuffer *src, GstBuffer *dest, GstVideoInfo dest_info, guint32 stride)
We usually avoid copying larger structure, even though valid. So instead we pass a const
pointer to VideoInfo.
> +{
> + GstVideoInfo src_info = dest_info;
> + GstVideoFrame src_frame, dest_frame;
> +
> + gst_libcamera_extrapolate_info(&src_info, stride);
> + src_info.size = gst_buffer_get_size(src);
> +
> + if (!gst_video_frame_map(&src_frame, &src_info, src, GST_MAP_READ))
> + goto invalid_buffer;
> +
> + if (!gst_video_frame_map(&dest_frame, &dest_info, dest, GST_MAP_WRITE)) {
> + gst_video_frame_unmap(&src_frame);
> + goto invalid_buffer;
> + }
> +
> + gst_video_frame_copy(&dest_frame, &src_frame);
This can fail, usually a programming error, but its nice to report it.
> +
> + gst_video_frame_unmap(&src_frame);
> + gst_video_frame_unmap(&dest_frame);
> +
> + return GST_FLOW_OK;
> +
> +invalid_buffer : {
> + GST_ERROR("Could not map buffer");
> + return GST_FLOW_ERROR;
> +}
> +}
> +
> /* Must be called with stream_lock held. */
> int GstLibcameraSrcState::processRequest()
> {
> @@ -292,11 +338,36 @@ int GstLibcameraSrcState::processRequest()
> GstFlowReturn ret = GST_FLOW_OK;
> gst_flow_combiner_reset(src_->flow_combiner);
>
> - for (GstPad *srcpad : srcpads_) {
> + for (gsize i = 0; i < src_->state->srcpads_.size(); i++) {
Why not just srcpads_.size() ?
Cause I think if "this" is not the same as src_->state, we have a problem.
> + GstPad *srcpad = src_->state->srcpads_[i];
idem.
> Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> GstBuffer *buffer = wrap->detachBuffer(stream);
>
> FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);
> + const StreamConfiguration &stream_cfg = src_->state->config_->at(i);
> + GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad);
> +
> + if (video_pool) {
Perhaps a comment explaining that we only set a pool when a copy is needed ?
> + GstBuffer *copy = NULL;
> + GstVideoInfo info = gst_libcamera_pad_get_video_info(srcpad);
> +
> + ret = gst_buffer_pool_acquire_buffer(video_pool, ©, NULL);
> + if (ret != GST_FLOW_OK) {
> + GST_ERROR("Failed to acquire buffer");
> + gst_buffer_unref(buffer);
> + return -EPIPE;
EPIPE paused the thread, this will lead to stall. If you want this to be fatal
you must post an error message on the bus (see GST_ELEMENT_ERROR() macro.
> + }
> +
> + ret = gst_libcamera_video_frame_copy(buffer, copy, info, stream_cfg.stride);
> + gst_buffer_unref(buffer);
> + if (ret != GST_FLOW_OK) {
> + GST_ERROR("Failed to copy buffer");
> + gst_buffer_unref(copy);
> + return -EPIPE;
Same.
> + }
> +
> + buffer = copy;
> + }
>
> if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) {
> GST_BUFFER_PTS(buffer) = wrap->pts_;
> @@ -497,13 +568,65 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> for (gsize i = 0; i < state->srcpads_.size(); i++) {
> GstPad *srcpad = state->srcpads_[i];
> const StreamConfiguration &stream_cfg = state->config_->at(i);
> + GstBufferPool *video_pool = NULL;
> + GstVideoInfo info;
> +
> + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
> +
> + gst_video_info_init(&info);
Not needed if you use from_caps() below ...
> + gst_video_info_from_caps(&info, caps);
> + gst_libcamera_pad_set_video_info(srcpad, info);
> +
> + /* 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;
> + gboolean need_pool = true;
Since you just use it for readability, can be const.
> + 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_buffer_pool_set_config(GST_BUFFER_POOL_CAST(video_pool), config);
> + GST_DEBUG_OBJECT(self, "Own pool config is %" GST_PTR_FORMAT, config);
Move the trace before the set, since you don't own the pointer afterward,
which is risky.
> + }
> +
> + if (!gst_buffer_pool_set_active(video_pool, true)) {
> + GST_ERROR_OBJECT(self, "Failed to active buffer pool");
Use GST_ELEMENT_ERROR() here.
> + gst_caps_unref(caps);
> + return false;
> + }
> + }
> + gst_query_unref(query);
> + }
>
> GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> - stream_cfg.stream());
> + stream_cfg.stream(), &info);
> g_signal_connect_swapped(pool, "buffer-notify",
> G_CALLBACK(gst_task_resume), self->task);
>
> gst_libcamera_pad_set_pool(srcpad, pool);
> + gst_libcamera_pad_set_video_pool(srcpad, video_pool);
See comment about passing a reference. Might be nicer to combine passing the pool
and the video info ?
>
> /* Clear all reconfigure flags. */
> gst_pad_check_reconfigure(srcpad);
> @@ -920,6 +1043,12 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad)
> auto end_iterator = pads.end();
> auto pad_iterator = std::find(begin_iterator, end_iterator, pad);
>
> + GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(pad);
> + if (video_pool) {
> + gst_buffer_pool_set_active(video_pool, false);
> + gst_object_unref(video_pool);
> + }
What about moving this in the pad, and simpy calling gst_libcamera_pad_set_pool(nullptr),
or pad_unset_pool() ?
> +
> if (pad_iterator != end_iterator) {
> g_object_unref(*pad_iterator);
> pads.erase(pad_iterator);
More information about the libcamera-devel
mailing list