[EXT] Re: [PATCH v3] gstreamer: Add GstVideoMeta support
Qi Hou
qi.hou at nxp.com
Wed Nov 20 11:04:54 CET 2024
Hi,
Thanks for all of your review. Suggestions are useful. I make a v4 patch. Please help review that version. I am also doing more local test.
Regards,
Qi Hou
-----Original Message-----
From: Nicolas Dufresne <nicolas at ndufresne.ca>
Sent: 2024年11月14日 3:20
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 v3] 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 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.
> *
> * 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.
> +{
> + 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);
> + 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