[EXT] Re: [PATCH v6] gstreamer: Add GstVideoMeta support
Qi Hou
qi.hou at nxp.com
Tue May 13 11:29:29 CEST 2025
Hi Nicolas,
Thank you for revising my patch line by line. I will submit v7 after I finish coding and testing.
Regards,
Qi Hou
-----Original Message-----
From: Nicolas Dufresne <nicolas at ndufresne.ca>
Sent: 2025年5月13日 5:06
To: Qi Hou <qi.hou at nxp.com>; libcamera-devel at lists.libcamera.org
Cc: Jared Hu <jared.hu at nxp.com>; Julien Vuillaumier <julien.vuillaumier at nxp.com>
Subject: [EXT] Re: [PATCH v6] gstreamer: Add GstVideoMeta support
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
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