[EXT] Re: [PATCH v2] gstreamer: Add GstVideoMeta support
Qi Hou
qi.hou at nxp.com
Wed Nov 13 08:34:12 CET 2024
Hi Laurent Pinchart and Nicolas,
Thank you both. I have sent out v3 for review. Please correct me if need improvement.
Regards,
Qi Hou
-----Original Message-----
From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Sent: 2024年11月13日 2:43
To: Nicolas Dufresne <nicolas at ndufresne.ca>
Cc: Qi Hou <qi.hou at nxp.com>; Kieran Bingham <kieran.bingham at ideasonboard.com>; libcamera-devel at lists.libcamera.org; Jared Hu <jared.hu at nxp.com>; Julien Vuillaumier <julien.vuillaumier at nxp.com>
Subject: Re: [EXT] Re: [PATCH v2] 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 Nicolas,
On Tue, Nov 12, 2024 at 01:27:12PM -0500, Nicolas Dufresne wrote:
> Le mardi 12 novembre 2024 à 07:33 +0200, Laurent Pinchart a écrit :
> > On Tue, Nov 12, 2024 at 02:06:44AM +0000, Qi Hou wrote:
> > > Hi Kieran,
> > >
> > > "gst_video_format_info_extrapolate_stride" is defined since
> > > gstreamer 1.22. I think it causes build fail on old compiler with
> > > gstreamer version lower than 1.22. Do I need to add
> > > GST_CHECK_VERSION(1, 22, 0) to uses this function for gstreamer
> > > version >=1.22.0, and fallback to uses other functions for gstreamer version <1.22.0 ?
> >
> > I'd like to keep supporting 1.18 if possible as it's still shipped
> > by Debian Bullseye. A version check is fine, it will clearly mark
> > the
>
> Fun fact, Debian Bullseye will never be updated to ship a newer GStreamer.
> Debian never updates major GStreamer version in stable.
I know. I didn't imply they would update.
> Though, it seems very
> up-to-date on security fixes, which I can see in some of the logs
> (these are fixes that upstream did not backport and release, because
> we only go back 2
> versions):
>
> https://meta/
> data.ftp-master.debian.org%2Fchangelogs%2F%2Fmain%2Fg%2Fgst-plugins-go
> od1.0%2Fgst-plugins-good1.0_1.18.4-2%2Bdeb11u2_changelog&data=05%7C02%
> 7Cqi.hou%40nxp.com%7C107b6b68583749d3275608dd0349e0ab%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C638670338070753380%7CUnknown%7CTWFpbGZsb3
> d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi
> TWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=qviS2QbkjVxChmr%2Bf%2BWgcV
> 13XPhKAduO7RaEumh3Tvg%3D&reserved=0
>
> My understanding is that this is EOL 5 years after the .0 release, so
> in August 2026. Is it your plan to drop it at the time ? Its quite
> unusable to see libraries keeping backward support for that long. But
> it would be nice to have this officially stated somewhere.
We try to support the current and previous versions of major distributions when possible. For Debian, this is also due to using bullseye in CI to test compilation with gcc 9 and gcc 10.
> > backward compatibility code, and allow us to remove it once we'll
> > bump the minimum GStreamer version.
>
> That function only depends on 1.18 internally, so yes, that's an option:
>
> #if < 1.22
>
> 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
>
> Don't forget to refer it and copy over the copyright:
>
>
> * 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 code is LGPL V2.
>
> > > On 2024年11月11日 17:30, Kieran Bingham wrote:
> > > > Quoting Hou Qi (2024-11-08 09:21:13)
> > > > > 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.
> > > >
> > > > Thanks for the v2, this looks like interesting work.
> > > >
> > > > Unfortunately the CI isn't clear on this one yet:
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%25
> > > > 2Fgitlab.freedesktop.org%2Fcamera%2Flibcamera%2F-%2Fjobs%2F66327
> > > > 653&data=05%7C02%7Cqi.hou%40nxp.com%7C107b6b68583749d3275608dd03
> > > > 49e0ab%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638670338070
> > > > 783724%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwL
> > > > jAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%
> > > > 7C%7C%7C&sdata=0YiVVZn3QlJQmdf8YElnAo%2B0flzOC53cjW7HRUQVx5w%3D&
> > > > reserved=0
> > > >
> > > > Any idea what's happening here? It looks like this only failed
> > > > on one
> > > > compiler:
> > > >
> > > > [443/641] Generating src/py/libcamera/py_gen_formats with a
> > > > custom command [444/641] Compiling C++ object
> > > > src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o
> > > > FAILED:
> > > > src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o
> > > > g++-9 -Isrc/gstreamer/libgstlibcamera.so.p -Isrc/gstreamer
> > > > g++-I../src/gstreamer -Iinclude -I../include -Iinclude/libcamera
> > > > g++-I/usr/include/gstreamer-1.0 -I/usr/include/orc-0.4
> > > > g++-I/usr/include/x86_64-linux-gnu -I/usr/include/glib-2.0
> > > > g++-I/usr/lib/x86_64-linux-gnu/glib-2.0/include
> > > > g++-fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall
> > > > g++-Winvalid-pch -Wextra -Werror -std=c++17 -O0 -g
> > > > g++-Wmissing-declarations -Wshadow -include
> > > > g++/builds/camera/libcamera/build/config.h -fPIC -pthread '-DVERSION="0.3.2+1-edf89091-nvm"' '-DPACKAGE="libcamera"'
> > > > g++-DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 -MD -MQ
> > > > g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -MF
> > > > g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o.d
> > > > g++-o src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o
> > > > g++-c ../src/gstreamer/gstlibcamerapool.cpp
> > > > ../src/gstreamer/gstlibcamerapool.cpp: In function ‘GstLibcameraPool* gst_libcamera_pool_new(GstLibcameraAllocator*, const libcamera::StreamConfiguration&, GstCaps*, gboolean)’:
> > > > ../src/gstreamer/gstlibcamerapool.cpp:151:13: error: ‘gst_video_format_info_extrapolate_stride’ was not declared in this scope; did you mean ‘gst_video_format_info_component’?
> > > > 151 | stride = gst_video_format_info_extrapolate_stride(info.finfo, k, stream_cfg.stride);
> > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > | gst_video_format_info_component
> > > > [445/641] Compiling C++ object
> > > > src/apps/qcam/qcam.p/viewfinder_qt.cpp.o
> > > >
> > > > So perhaps we're hitting a requirement to have a specific gstreamer version that isn't being catered for ?
> > > >
> > > > --
> > > > Kieran
> > > >
> > > > > Signed-off-by: Hou Qi <qi.hou at nxp.com>
> > > > > ---
> > > > > src/gstreamer/gstlibcamerapool.cpp | 28 ++++++++++++++++++++++++----
> > > > > src/gstreamer/gstlibcamerapool.h | 2 +-
> > > > > src/gstreamer/gstlibcamerasrc.cpp | 14 +++++++++++++-
> > > > > 3 files changed, 38 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/src/gstreamer/gstlibcamerapool.cpp
> > > > > b/src/gstreamer/gstlibcamerapool.cpp
> > > > > index 9cd7eccb..6593b3ca 100644
> > > > > --- a/src/gstreamer/gstlibcamerapool.cpp
> > > > > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > > > > @@ -135,16 +135,36 @@
> > > > > 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();
> > > > > +
> > > > > + if (caps && gst_video_info_from_caps(&info, caps)) {
> > > > > + guint k, stride;
> > > > > + gsize offset = 0;
> > > > > + for (k = 0; k < GST_VIDEO_INFO_N_PLANES(&info); k++) {
> > > > > + stride = gst_video_format_info_extrapolate_stride(info.finfo, k, stream_cfg.stride);
> > > > > + info.stride[k] = stride;
> > > > > + info.offset[k] = offset;
> > > > > + offset += stride * GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT(info.finfo, k, GST_VIDEO_INFO_HEIGHT(&info));
> > > > > + }
> > > > > + } else
> > > > > + add_video_meta = false;
I think there are a few coding style issues that checkstyle.py could have caught. See https://libcamera.org/coding-style.html#tools for more information, especially the last paragraph that explains how to use the git commit hook.
> > > > > +
> > > > > + 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);
> > > > > + 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);
> > > > > + 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