[PATCH v8] gstreamer: Add GstVideoMeta support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 16 14:58:28 CEST 2025
On Fri, May 16, 2025 at 02:54:27PM +0200, Laurent Pinchart wrote:
> On Fri, May 16, 2025 at 12:16:50PM +0200, Nicolas Dufresne wrote:
> > Hi,
> >
> > Le vendredi 16 mai 2025 à 16:30 +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>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > I'm happy with this version.
>
> Unfortunately CI isn't as happy as you :-(
>
> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/76508291
>
> ../src/gstreamer/gstlibcamerasrc.cpp: In function ‘GstFlowReturn gst_libcamera_video_frame_copy(GstBuffer*, GstBuffer*, const GstVideoInfo*, guint32)’:
> ../src/gstreamer/gstlibcamerasrc.cpp:301:40: error: invalid conversion from ‘const GstVideoInfo*’ {aka ‘const _GstVideoInfo*’} to ‘GstVideoInfo*’ {aka ‘_GstVideoInfo*’} [-fpermissive]
> 301 | if (!gst_video_frame_map(&dest_frame, dest_info, dest, GST_MAP_WRITE)) {
> | ^~~~~~~~~
> | |
> | const GstVideoInfo* {aka const _GstVideoInfo*}
>
> The issue was fixed in GStreamer 1.19.3, in
>
> commit abb026ec6ab1617b44de69e9a251317592eee755
> Author: Marijn Suijten <marijns95 at gmail.com>
> Date: Mon Jan 4 23:25:10 2021 +0100
>
> gl,video: Make ptrs to VideoInfo and (GL)AllocationParams immutable
>
> These parameters are incorrectly regarded as mutable in G-IR making them
> "incompatible" with languages that are explicit about mutability like
> Rust. In order to clean up the code and expected API there, update the
> signatures here, right at the source (instead of overriding them in
> Gir.toml and hoping for the best).
>
> Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/1005>
>
> that changed the function prototype as follows:
>
> GST_VIDEO_API
> -gboolean gst_video_frame_map (GstVideoFrame *frame, GstVideoInfo *info,
> +gboolean gst_video_frame_map (GstVideoFrame *frame, const GstVideoInfo *info,
> GstBuffer *buffer, GstMapFlags flags);
>
> libcamera currently requires GStreamer 1.14.0 or newer. We could upgrade
> to a newer version, but I'd like to support Debian Bullseye until it's
> end of life, which is scheduled for August 2026, and that comes with
> GStreamer 1.18.
>
> I think we can just const_cast<> the dest_info argument to
> gst_video_frame_map(), with a comment that explains what's going on:
>
> /*
> * Before v1.19.3, the gst_video_frame_map() function took a non-const
> * info argument, even if it didn't modify the pointer structure. To
> * avoid breaking compilation, cast dest_info to a non-const pointer.
> * This should be removed when dropping support for older GStreamer
> * versions.
> */
> if (!gst_video_frame_map(&dest_frame, const_cast<GstVideoInfo>(dest_info),
> dest, GST_MAP_WRITE)) {
>
> I could make that change when applying, but I can't easily test it at
> the moment. Hou, would you mind fixing and testing this, and send a new
> version ?
>
> As a new version is needed, I also have a few cosmetic comments, please
> see below. There's also one question for Nicolas.
Also, please look at the CI linter warnings from
https://gitlab.freedesktop.org/camera/libcamera/-/jobs/76508306 and
fix issues if they're not false positives.
> > Tested-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> >
> > > ---
> > > src/gstreamer/gstlibcamera-utils.cpp | 33 ++++++
> > > src/gstreamer/gstlibcamera-utils.h | 5 +
> > > src/gstreamer/gstlibcamerapad.cpp | 31 ++++++
> > > src/gstreamer/gstlibcamerapad.h | 8 ++
> > > src/gstreamer/gstlibcamerapool.cpp | 15 ++-
> > > src/gstreamer/gstlibcamerapool.h | 3 +-
> > > src/gstreamer/gstlibcamerasrc.cpp | 147 ++++++++++++++++++++++++++-
> > > 7 files changed, 238 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > index 2edebba0..bf12cb07 100644
> > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > @@ -599,6 +599,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. */
>
> Small style issue:
>
> /*
> * 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.
> */
>
> Same below where applicable.
>
> > > +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 4978987c..5f4e8a0f 100644
> > > --- a/src/gstreamer/gstlibcamera-utils.h
> > > +++ b/src/gstreamer/gstlibcamera-utils.h
> > > @@ -36,6 +36,11 @@ 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);
> > >
> > > /**
> > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> > > index 7b22aebe..3bc2bc87 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, const GstVideoInfo *info)
> > > +{
> > > + 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..f98b8a7f 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, const 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..8278144f 100644
> > > --- a/src/gstreamer/gstlibcamerapool.cpp
> > > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > > @@ -134,8 +134,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 +157,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 5e9e843d..4b81c94e 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -268,6 +268,58 @@ 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. */
>
> Sentences should start with a capital letter, and end with a period:
>
> /* 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, const GstVideoInfo *dest_info, guint32 stride)
> > > +{
> > > + 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)) {
> > > + GST_ERROR("Could not map buffer");
> > > + goto error;
>
> You can just
>
> return GST_FLOW_ERROR;
>
> here. Same below, and drop the "error:" label.
>
> > > + }
> > > +
> > > + if (!gst_video_frame_map(&dest_frame, dest_info, dest, GST_MAP_WRITE)) {
> > > + GST_ERROR("Could not map buffer");
> > > + gst_video_frame_unmap(&src_frame);
> > > + goto error;
> > > + }
> > > +
> > > + if (!gst_video_frame_copy(&dest_frame, &src_frame)) {
> > > + GST_ERROR("Could not copy frame");
> > > + gst_video_frame_unmap(&src_frame);
> > > + gst_video_frame_unmap(&dest_frame);
> > > + goto error;
> > > + }
> > > +
> > > + gst_video_frame_unmap(&src_frame);
> > > + gst_video_frame_unmap(&dest_frame);
> > > +
> > > + return GST_FLOW_OK;
> > > +
> > > +error:
> > > + return GST_FLOW_ERROR;
> > > +}
> > > +
> > > /* Must be called with stream_lock held. */
> > > int GstLibcameraSrcState::processRequest()
> > > {
> > > @@ -292,11 +344,41 @@ int GstLibcameraSrcState::processRequest()
> > > GstFlowReturn ret = GST_FLOW_OK;
> > > gst_flow_combiner_reset(src_->flow_combiner);
> > >
> > > - for (GstPad *srcpad : srcpads_) {
> > > + for (gsize i = 0; i < srcpads_.size(); i++) {
> > > + GstPad *srcpad = srcpads_[i];
> > > 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 = config_->at(i);
> > > + GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad);
> > > +
> > > + if (video_pool) {
> > > + /* Only set video pool when a copy is needed */
> > > + GstBuffer *copy = NULL;
> > > + const GstVideoInfo info = gst_libcamera_pad_get_video_info(srcpad);
> > > +
> > > + ret = gst_buffer_pool_acquire_buffer(video_pool, ©, NULL);
> > > + if (ret != GST_FLOW_OK) {
> > > + gst_buffer_unref(buffer);
> > > + GST_ELEMENT_ERROR(src_, RESOURCE, SETTINGS,
> > > + ("Failed to acquire buffer"),
> > > + ("GstLibcameraSrcState::processRequest() failed: %s", g_strerror(-
> > > ret)));
> > > + return -EPIPE;
> > > + }
> > > +
> > > + ret = gst_libcamera_video_frame_copy(buffer, copy, &info, stream_cfg.stride);
> > > + gst_buffer_unref(buffer);
> > > + if (ret != GST_FLOW_OK) {
> > > + gst_buffer_unref(copy);
> > > + GST_ELEMENT_ERROR(src_, RESOURCE, SETTINGS,
> > > + ("Failed to copy buffer"),
> > > + ("GstLibcameraSrcState::processRequest() failed: %s", g_strerror(-
> > > ret)));
> > > + return -EPIPE;
> > > + }
> > > +
> > > + buffer = copy;
> > > + }
> > >
> > > if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) {
> > > GST_BUFFER_PTS(buffer) = wrap->pts_;
> > > @@ -499,13 +581,68 @@ 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, transfer[i]);
> > > +
> > > + 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) {
>
> Nicolas, as copying frames is very expensive, should we log a message
> somewhere to warn the user ? I'm concerned we'll get bug reports for bad
> performance, and I'd like a way to know that copies are happening.
>
> > > + 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);
> > > + }
> > > +
> > > + 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);
> > > + }
> > >
> > > 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);
> > >
> > > /* Clear all reconfigure flags. */
> > > gst_pad_check_reconfigure(srcpad);
> > > @@ -922,6 +1059,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);
> > > + }
> > > +
> > > if (pad_iterator != end_iterator) {
> > > g_object_unref(*pad_iterator);
> > > pads.erase(pad_iterator);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list