| Message ID | 20241113072947.1579075-1-qi.hou@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@collabora.com> > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > + * Copyright (C) <2007> David A. Schleef <ds@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@collabora.com> > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > + * Copyright (C) <2007> David A. Schleef <ds@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); >
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@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@collabora.com> > > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > > + * Copyright (C) <2007> David A. Schleef <ds@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@collabora.com> > > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > > + * Copyright (C) <2007> David A. Schleef <ds@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); > > >
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@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@collabora.com> > > > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > > > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > > > + * Copyright (C) <2007> David A. Schleef <ds@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@collabora.com> > > > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > > > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > > > + * Copyright (C) <2007> David A. Schleef <ds@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); > > >
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@ndufresne.ca> Sent: 2024年11月14日 3:20 To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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@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@collabora.com> > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > + * Copyright (C) <2007> David A. Schleef <ds@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@collabora.com> > + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> > + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > + * Copyright (C) <2007> David A. Schleef <ds@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); >
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@collabora.com> + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> + * Copyright (C) <2007> David A. Schleef <ds@schleef.org> * * 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) +{ + 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@collabora.com> + * Copyright (C) <1999> Erik Walthinsen <omega@cse.ogi.edu> + * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> + * Copyright (C) <2007> David A. Schleef <ds@schleef.org> * * 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); +#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(); + + 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++) { +#if !GST_CHECK_VERSION(1, 22, 0) + 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)); + } + } else + add_video_meta = false; + + 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);
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@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(-)