[PATCH v3] gstreamer: Add GstVideoMeta support
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 14 10:21:59 CET 2024
Quoting Nicolas Dufresne (2024-11-13 19:19:51)
> Hi,
>
> Le mercredi 13 novembre 2024 à 16:29 +0900, Hou Qi a écrit :
> > GStreamer video-info calculated stride and offset may differ from
> > those used by the camera.
> >
> > This patch enhances downstream plugin's support for videometa by
> > adding videometa support for libcamerasrc. It ensures that when
> > downstream plugin supports videometa by allocation query, libcamerasrc
> > also attaches videometa to buffer, preventing discrepancies
> > between the stride and offset calculated by video-info and camera.
> >
> > Signed-off-by: Hou Qi <qi.hou at nxp.com>
> > ---
> > src/gstreamer/gstlibcamera-utils.cpp | 28 ++++++++++++++++++++++++
> > src/gstreamer/gstlibcamera-utils.h | 7 ++++++
> > src/gstreamer/gstlibcamerapool.cpp | 32 ++++++++++++++++++++++++----
> > src/gstreamer/gstlibcamerapool.h | 2 +-
> > src/gstreamer/gstlibcamerasrc.cpp | 14 +++++++++++-
> > 5 files changed, 77 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index 732987ef..91de1d44 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -2,6 +2,9 @@
> > /*
> > * Copyright (C) 2020, Collabora Ltd.
> > * Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > + * 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>
>
> I'll leave that to the upper maintainers to evaluate, but it would be nice to
> identify that these are only needed as long as
> gst_libcamera_extrapolate_stride() copy is there. As soon as Trixie becomes
> stable, we'll bump to GStreamer 1.22 and remove it, would be nice to remove the
> spurious copyright.
What about putting it in a comment banner directly adjacent to the
function with an explanation that the function has been imported
directly from the gstreamer project to support backwards compatibility
and will be removed (the function and the comment banner) when the the
older version is no longer supported ?
Then the copyright notice is directly adjacent to the code it relates
to, and it's easy to see and remove it all in one go.
Another alternative would be to create a new
gstlibcamera-compatibility.cpp and put it there on it's own - but I
think that's overkill, so keeping it in -utils is fine with me.
>
> > *
> > * GStreamer libcamera Utility Function
> > */
> > @@ -591,6 +594,31 @@ gst_task_resume(GstTask *task)
> > }
> > #endif
> >
> > +#if !GST_CHECK_VERSION(1, 22, 0)
> > +gint gst_libcamera_extrapolate_stride(const GstVideoFormatInfo *finfo, gint plane, gint stride)
>
> Because its under an ifdef, it is guarantied not to clash at built time, and the
> linker will always resolve it locally. Keep the mainline namespace, it will
> simplify the code later.
Yes, I agree - lets keep this as
gst_video_format_info_extrapolate_stride please.
>
> > +{
> > + 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..14bf6664 100644
> > --- a/src/gstreamer/gstlibcamera-utils.h
> > +++ b/src/gstreamer/gstlibcamera-utils.h
> > @@ -2,6 +2,9 @@
> > /*
> > * Copyright (C) 2020, Collabora Ltd.
> > * Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > + * 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>
>
> Not needed.
>
> > *
> > * GStreamer libcamera Utility Functions
> > */
> > @@ -35,6 +38,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_libcamera_extrapolate_stride(const GstVideoFormatInfo *finfo, gint plane, gint stride);
>
> Apply namespace comment here too.
>
> > +#endif
> > std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret);
> >
> > /**
> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> > index 9cd7eccb..315f08ac 100644
> > --- a/src/gstreamer/gstlibcamerapool.cpp
> > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > @@ -135,16 +135,40 @@ gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> > }
> >
> > GstLibcameraPool *
> > -gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, const StreamConfiguration &stream_cfg,
> > + GstCaps *caps, gboolean add_video_meta)
> > {
> > auto *pool = GST_LIBCAMERA_POOL(g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr));
> > + GstVideoInfo info;
> >
> > pool->allocator = GST_LIBCAMERA_ALLOCATOR(g_object_ref(allocator));
> > - pool->stream = stream;
> > -
> > - gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> > + pool->stream = stream_cfg.stream();
>
> nit: Is pool->stream const ? C++ and C are pretty permissive, but storing a non-
> const pointer obtained from a const object is a bit of a stretch.
>
> > +
> > + if (caps && gst_video_info_from_caps(&info, caps)) {
>
> This needs some work, caps will never be null. Now, gst_video_info_from_caps()
> will work for image/ and video/ type, in that case the format is set to ENCODED
> and a video meta must not be appended. That includes bayer format, for which we
> only support matching display and storage dimensions (since arbitrary cropping
> of bayer is a bad idea).
>
> In short, don't get caps, and be verbose if gst_video_info_from_caps() fails,
> that is not expected.
>
> > + guint k, stride;
> > + gsize offset = 0;
> > + for (k = 0; k < GST_VIDEO_INFO_N_PLANES(&info); k++) {
> > +#if !GST_CHECK_VERSION(1, 22, 0)
>
> Not needed with the suggestion I just made.
>
> > + stride = gst_libcamera_extrapolate_stride(info.finfo, k, stream_cfg.stride);
> > +#else
> > + stride = gst_video_format_info_extrapolate_stride(info.finfo, k, stream_cfg.stride);
> > +#endif
> > + info.stride[k] = stride;
> > + info.offset[k] = offset;
> > + offset += stride * GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT(info.finfo, k, GST_VIDEO_INFO_HEIGHT(&info));
>
> Please add a comment that this should be updated if tiled formats get added in
> the future. For anything single plane we don't care of course.
>
> > + }
> > + } else
> > + add_video_meta = false;
>
> Not sure yet why sometimes you don't want to add it.
>
> > +
> > + gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream_cfg.stream());
> > for (gsize i = 0; i < pool_size; i++) {
> > GstBuffer *buffer = gst_buffer_new();
> > + if (add_video_meta) {
> > + 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);
> > + }
> > pool->queue->push_back(buffer);
> > }
> >
> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> > index 2a7a9c77..8ad87cab 100644
> > --- a/src/gstreamer/gstlibcamerapool.h
> > +++ b/src/gstreamer/gstlibcamerapool.h
> > @@ -21,7 +21,7 @@
> > G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_LIBCAMERA, POOL, GstBufferPool)
> >
> > GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> > - libcamera::Stream *stream);
> > + const libcamera::StreamConfiguration &stream_cfg, GstCaps *caps, gboolean add_video_meta);
> >
> > libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self);
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 8efa25f4..c05a31e7 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -497,9 +497,21 @@ 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);
> > + GstQuery *query = NULL;
> > + gboolean add_video_meta = false;
> > +
> > + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
> > + gst_libcamera_framerate_to_caps(caps, element_caps);
> > +
> > + query = gst_query_new_allocation(caps, false);
>
> My hate for boolean parameters is strong enough that I'd like to suggest:
>
> bool need_pool = false;
> query = gst_query_new_allocation(caps, need_pool);
bool kieran_agrees = true;
ret = send_response(kieran_agrees);
> > + if (!gst_pad_peer_query(srcpad, query))
> > + GST_DEBUG_OBJECT(self, "didn't get downstream ALLOCATION hints");
> > + else
> > + add_video_meta = gst_query_find_allocation_meta(query, GST_VIDEO_META_API_TYPE,
> > NULL);
>
> Its not sufficient to not add video meta. If downstream does not support
> VideoMeta and the stride does not match what you from just turning the caps into
> VideoInfo, you must bounce these buffers to system memory. For that reason, you
> likely want to handle the stride extrapolation here and pass VideoInfo to the
> pool instead. If the stride miss-match, you can then create a generic VideoPool
> using the caps, and later own you will bounce the memory using
> gst_video_frame_copy() (which can handle stride miss-match).
>
> > + gst_query_unref(query);
> >
> > GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> > - stream_cfg.stream());
> > + stream_cfg, caps, add_video_meta);
> > g_signal_connect_swapped(pool, "buffer-notify",
> > G_CALLBACK(gst_task_resume), self->task);
> >
>
More information about the libcamera-devel
mailing list