[PATCH v3] gstreamer: Add GstVideoMeta support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 14 11:56:48 CET 2024
On Thu, Nov 14, 2024 at 09:21:59AM +0000, Kieran Bingham wrote:
> 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.
Agreed, a new file is overkill, we can just add the copyright comment
directly above the function, within the #ifdef version guard.
> > > *
> > > * 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);
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list