| Message ID | 20241108092113.3769844-1-qi.hou@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Qi, 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://gitlab.freedesktop.org/camera/libcamera/-/jobs/66327653 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 -I../src/gstreamer -Iinclude -I../include -Iinclude/libcamera -I/usr/include/gstreamer-1.0 -I/usr/include/orc-0.4 -I/usr/include/x86_64-linux-gnu -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c++17 -O0 -g -Wmissing-declarations -Wshadow -include /builds/camera/libcamera/build/config.h -fPIC -pthread '-DVERSION="0.3.2+1-edf89091-nvm"' '-DPACKAGE="libcamera"' -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 -MD -MQ src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -MF src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o.d -o src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -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@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; > + > + 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); > > -- > 2.34.1 >
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 ? Regards, Qi Hou -----Original Message----- From: Kieran Bingham <kieran.bingham@ideasonboard.com> Sent: 2024年11月11日 17:30 To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Qi Hou <qi.hou@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: [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 Qi, 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://gitlab.freedesktop.org/camera/libcamera/-/jobs/66327653 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 -Winvalid-pch g++-Wextra -Werror -std=c++17 -O0 -g -Wmissing-declarations -Wshadow g++-include /builds/camera/libcamera/build/config.h -fPIC -pthread g++'-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 -o g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -c g++../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@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; > + > + 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); > > -- > 2.34.1 >
Hi Qi, 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 backward compatibility code, and allow us to remove it once we'll bump the minimum GStreamer version. > 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://gitlab.freedesktop.org/camera/libcamera/-/jobs/66327653 > > > > 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 -Winvalid-pch > > g++-Wextra -Werror -std=c++17 -O0 -g -Wmissing-declarations -Wshadow > > g++-include /builds/camera/libcamera/build/config.h -fPIC -pthread > > g++'-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 -o > > g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -c > > g++../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@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; > > > + > > > + 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); > > >
Le mardi 12 novembre 2024 à 07:33 +0200, Laurent Pinchart a écrit : > Hi Qi, > > 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. 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://metadata.ftp-master.debian.org/changelogs//main/g/gst-plugins-good1.0/gst-plugins-good1.0_1.18.4-2+deb11u2_changelog 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. > 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@cse.ogi.edu> * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> * Copyright (C) 2007 David A. Schleef <ds@schleef.org> This code is LGPL V2. Nicolas > > > 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://gitlab.freedesktop.org/camera/libcamera/-/jobs/66327653 > > > > > > 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 -Winvalid-pch > > > g++-Wextra -Werror -std=c++17 -O0 -g -Wmissing-declarations -Wshadow > > > g++-include /builds/camera/libcamera/build/config.h -fPIC -pthread > > > g++'-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 -o > > > g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -c > > > g++../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@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; > > > > + > > > > + 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); > > > > >
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://metadata.ftp-master.debian.org/changelogs//main/g/gst-plugins-good1.0/gst-plugins-good1.0_1.18.4-2+deb11u2_changelog > > 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@cse.ogi.edu> > * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > * Copyright (C) 2007 David A. Schleef <ds@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://gitlab.freedesktop.org/camera/libcamera/-/jobs/66327653 > > > > > > > > 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 -Winvalid-pch > > > > g++-Wextra -Werror -std=c++17 -O0 -g -Wmissing-declarations -Wshadow > > > > g++-include /builds/camera/libcamera/build/config.h -fPIC -pthread > > > > g++'-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 -o > > > > g++src/gstreamer/libgstlibcamera.so.p/gstlibcamerapool.cpp.o -c > > > > g++../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@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); > > > > >
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@ideasonboard.com> Sent: 2024年11月13日 2:43 To: Nicolas Dufresne <nicolas@ndufresne.ca> Cc: Qi Hou <qi.hou@nxp.com>; Kieran Bingham <kieran.bingham@ideasonboard.com>; libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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@cse.ogi.edu> > * Library <2002> Ronald Bultje <rbultje@ronald.bitfreak.net> > * Copyright (C) 2007 David A. Schleef <ds@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@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
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; + + 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/gstlibcamerapool.cpp | 28 ++++++++++++++++++++++++---- src/gstreamer/gstlibcamerapool.h | 2 +- src/gstreamer/gstlibcamerasrc.cpp | 14 +++++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-)