[{"id":3683,"web_url":"https://patchwork.libcamera.org/comment/3683/","msgid":"<20200212001625.GQ20823@pendragon.ideasonboard.com>","date":"2020-02-12T00:16:25","subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> This is needed to track the livetime of the FrameBufferAllocator in relation to\n\ns/livetime/lifetime/\n\n> the GstBuffer/GstMemory objects travelling inside GStreamer.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++\n> src/gstreamer/gstlibcameraallocator.h | 29 +++\n> src/gstreamer/gstlibcamerapool.cpp | 109 +++++++++++\n> src/gstreamer/gstlibcamerapool.h | 27 +++\n> src/gstreamer/meson.build | 11 +-\n> 5 files changed, 413 insertions(+), 3 deletions(-)\n> create mode 100644 src/gstreamer/gstlibcameraallocator.cpp\n> create mode 100644 src/gstreamer/gstlibcameraallocator.h\n> create mode 100644 src/gstreamer/gstlibcamerapool.cpp\n> create mode 100644 src/gstreamer/gstlibcamerapool.h\n> \n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> new file mode 100644\n> index 0000000..0f248b4\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> @@ -0,0 +1,240 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcameraallocator.cpp - GStreamer Custom Allocator\n> + */\n> +\n> +#include \"gstlibcameraallocator.h\"\n> +#include \"gstlibcamera-utils.h\"\n> +\n> +#include \n> +#include \n> +#include \n\nCould you please keep those headers alphabetically sorted ?\n\n> +\n> +using namespace libcamera;\n> +\n> +/***********************************************************************/\n> +/* Internal object for tracking memories associated with a FrameBuffer */\n> +/***********************************************************************/\n\nSame comment as before regarding this, a doxygen-style comment with a\ntiny bit of doc may be useful for inexperienced readers such as your\ndevoted reviewer :-) You could combine the next comment with it.\n\n> +\n> +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);\n\nIt would be nice to avoid forward declarations, but it may not be worth\nit.\n\n> +\n> +/* This internal object is used to track the outstanding GstMemory object that\n\ns/object/objects/\n\n> + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when\n> + * al all outstranging GstMemory have returned.\n\ns/al all/all/ ?\ns/outstranging/outstanding/ ?\n\n> + */\n> +struct FrameWrap {\n> +\tFrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> +\t\t gpointer stream);\n> +\t~FrameWrap();\n> +\n> +\tvoid AcquirePlane() { ++outstanding_planes_; }\n> +\tbool ReleasePlane() { return --outstanding_planes_ == 0; }\n\nMethods should start with a lowercase letter (camelCase, not CamelCase\nas for type names).\n\n> +\n> +\tgpointer stream_;\n> +\tFrameBuffer *buffer_;\n> +\tstd::vector planes_;\n> +\tgint outstanding_planes_;\n\noutstandingPlanes_ as it's a C++ class ?\n\n> +};\n> +\n> +static GQuark\n> +gst_libcamera_frame_quark(void)\n\nHow about making this a static function of the FrameWrap class ?\n\n> +{\n> +\tstatic gsize frame_quark = 0;\n> +\n> +\tif (g_once_init_enter(&frame_quark)) {\n> +\t\tGQuark quark = g_quark_from_string(\"GstLibCameraFrameWrap\");\n> +\t\tg_once_init_leave(&frame_quark, quark);\n> +\t}\n> +\n> +\treturn frame_quark;\n> +}\n> +\n> +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> +\t\t gpointer stream)\n> +\n> +\t: stream_(stream),\n> +\t buffer_(buffer),\n> +\t outstanding_planes_(0)\n> +{\n> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> +\t\tGstMemory *mem = gst_fd_allocator_alloc(allocator,\n> +\t\t\t\t\t\t\tplane.fd.fd(),\n> +\t\t\t\t\t\t\tplane.length,\n> +\t\t\t\t\t\t\tGST_FD_MEMORY_FLAG_DONT_CLOSE);\n> +\t\tgst_mini_object_set_qdata(GST_MINI_OBJECT(mem),\n> +\t\t\t\t\t gst_libcamera_frame_quark(),\n> +\t\t\t\t\t this, NULL);\n> +\t\tGST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;\n> +\t\tg_object_unref(mem->allocator);\n> +\t\tplanes_.push_back(mem);\n> +\t}\n> +}\n> +\n> +FrameWrap::~FrameWrap()\n> +{\n> +\tfor (GstMemory *mem : planes_) {\n> +\t\tGST_MINI_OBJECT(mem)->dispose = nullptr;\n> +\t\tg_object_ref(mem->allocator);\n> +\t\tgst_memory_unref(mem);\n> +\t}\n> +}\n> +\n> +/***********************************/\n> +/* The GstAllocator implementation */\n> +/***********************************/\n> +struct _GstLibcameraAllocator {\n> +\tGstDmaBufAllocator parent;\n> +\tFrameBufferAllocator *fb_allocator;\n> +\t/* A hash table using Stream pointer as key and returning a GQueue of\n> +\t * FrameWrap. */\n> +\tGHashTable *pools;\n> +};\n> +\n> +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> +\t GST_TYPE_DMABUF_ALLOCATOR);\n> +\n> +static gboolean\n> +gst_libcamera_allocator_release(GstMiniObject *mini_object)\n> +{\n> +\tGstMemory *mem = GST_MEMORY_CAST(mini_object);\n> +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);\n> +\tGST_OBJECT_LOCKER(self);\n> +\tgpointer data = gst_mini_object_get_qdata(mini_object,\n> +\t\t\t\t\t\t gst_libcamera_frame_quark());\n> +\tauto *frame = (FrameWrap *)data;\n\nCould you use reinterpret_cast(data) here ? And maybe\nreplace auto with FrameWrap as it's short.\n\nIn general, please use static_cast or reinterpret_cast as appropriate,\nas they offer more compile-time protection that C-style casts. I won't\ncomment on other occurrences.\n\n> +\n> +\tgst_memory_ref(mem);\n> +\n> +\tif (frame->ReleasePlane()) {\n> +\t\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);\n> +\t\tg_return_val_if_fail(pool, TRUE);\n> +\t\tg_queue_push_tail(pool, frame);\n> +\t}\n> +\n> +\t/* Keep last in case we are holding on the last allocator ref */\n> +\tg_object_unref(mem->allocator);\n> +\n> +\t/* Rreturns FALSE so that our mini object isn't freed */\n\ns/Rreturns/Return/\n\n> +\treturn FALSE;\n> +}\n> +\n> +static void\n> +gst_libcamera_allocator_free_pool(gpointer data)\n> +{\n> +\tGQueue *queue = (GQueue *)data;\n> +\tFrameWrap *frame;\n> +\n> +\twhile ((frame = (FrameWrap *)g_queue_pop_head(queue))) {\n> +\t\tg_warn_if_fail(frame->outstanding_planes_ == 0);\n> +\t\tdelete frame;\n> +\t}\n> +\n> +\tg_queue_free(queue);\n> +}\n> +\n> +static void\n> +gst_libcamera_allocator_init(GstLibcameraAllocator *self)\n> +{\n> +\tself->pools = g_hash_table_new_full(NULL, NULL, NULL,\n> +\t\t\t\t\t gst_libcamera_allocator_free_pool);\n> +\tGST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);\n> +}\n> +\n> +static void\n> +gst_libcamera_allocator_dispose(GObject *object)\n> +{\n> +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> +\n> +\tif (self->pools) {\n> +\t\tg_hash_table_unref(self->pools);\n> +\t\tself->pools = NULL;\n> +\t}\n> +\n> +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);\n> +}\n> +\n> +static void\n> +gst_libcamera_allocator_finalize(GObject *object)\n> +{\n> +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> +\n> +\tdelete self->fb_allocator;\n> +\n> +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> +}\n> +\n> +static void\n> +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)\n> +{\n> +\tauto *allocator_class = GST_ALLOCATOR_CLASS(klass);\n> +\tauto *object_class = G_OBJECT_CLASS(klass);\n> +\n> +\tobject_class->dispose = gst_libcamera_allocator_dispose;\n> +\tobject_class->finalize = gst_libcamera_allocator_finalize;\n> +\tallocator_class->alloc = NULL;\n> +}\n> +\n> +GstLibcameraAllocator *\n> +gst_libcamera_allocator_new(std::shared_ptr camera)\n> +{\n> +\tauto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> +\t\t\t\t\t\t\t nullptr);\n> +\n> +\tself->fb_allocator = FrameBufferAllocator::create(camera);\n> +\tfor (Stream *stream : camera->streams()) {\n> +\t\tgint ret;\n> +\n> +\t\tret = self->fb_allocator->allocate(stream);\n> +\t\tif (ret == 0)\n> +\t\t\treturn nullptr;\n> +\n> +\t\tGQueue *pool = g_queue_new();\n> +\t\tfor (const std::unique_ptr &buffer :\n> +\t\t self->fb_allocator->buffers(stream)) {\n> +\t\t\tauto *fb = new FrameWrap(GST_ALLOCATOR(self),\n> +\t\t\t\t\t\t buffer.get(), stream);\n> +\t\t\tg_queue_push_tail(pool, fb);\n> +\t\t}\n> +\n> +\t\tg_hash_table_insert(self->pools, stream, pool);\n> +\t}\n> +\n> +\treturn self;\n> +}\n> +\n> +bool\n> +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> +\t\t\t\t Stream *stream, GstBuffer *buffer)\n> +{\n> +\tGST_OBJECT_LOCKER(self);\n> +\n> +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> +\tg_return_val_if_fail(pool, false);\n> +\n> +\tauto *frame = (FrameWrap *)g_queue_pop_head(pool);\n> +\tif (!frame)\n> +\t\treturn false;\n> +\n> +\tfor (GstMemory *mem : frame->planes_) {\n> +\t\tframe->AcquirePlane();\n> +\t\tgst_buffer_append_memory(buffer, mem);\n> +\t\tg_object_ref(mem->allocator);\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +gsize\n> +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> +\t\t\t\t Stream *stream)\n> +{\n> +\tGST_OBJECT_LOCKER(self);\n> +\n> +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> +\tg_return_val_if_fail(pool, false);\n> +\n> +\treturn pool->length;\n> +}\n> diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> new file mode 100644\n> index 0000000..f2a0f58\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcameraallocator.h\n> @@ -0,0 +1,29 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcameraallocator.h - GStreamer Custom Allocator\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__\n> +#define __GST_LIBCAMERA_ALLOCATOR_H__\n> +\n> +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()\n> +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> +\t\t GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)\n> +\n> +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr camera);\n> +\n> +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> +\t\t\t\t\t libcamera::Stream *stream,\n> +\t\t\t\t\t GstBuffer *buffer);\n> +\n> +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> +\t\t\t\t\t libcamera::Stream *stream);\n> +\n> +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */\n> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> new file mode 100644\n> index 0000000..f84d1d6\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamerapool.cpp\n> @@ -0,0 +1,109 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamerapool.cpp - GStreamer Buffer Pool\n> + */\n> +\n> +#include \"gstlibcamerapool.h\"\n> +#include \"gstlibcamera-utils.h\"\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +struct _GstLibcameraPool {\n> +\tGstBufferPool parent;\n> +\n> +\tGstAtomicQueue *queue;\n> +\tGstLibcameraAllocator *allocator;\n> +\tStream *stream;\n> +};\n> +\n> +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);\n> +\n> +static GstFlowReturn\n> +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,\n> +\t\t\t\t GstBufferPoolAcquireParams *params)\n> +{\n> +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> +\tGstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);\n> +\tif (!buf)\n> +\t\treturn GST_FLOW_ERROR;\n> +\n> +\tif (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))\n> +\t\treturn GST_FLOW_ERROR;\n> +\n> +\t*buffer = buf;\n> +\treturn GST_FLOW_OK;\n> +}\n> +\n> +static void\n> +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> +{\n> +\tGstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);\n> +\n> +\t/* Clears all the memories and only pool the GstBuffer objects */\n> +\tgst_buffer_remove_all_memory(buffer);\n> +\tklass->reset_buffer(pool, buffer);\n> +\tGST_BUFFER_FLAGS(buffer) = 0;\n> +}\n> +\n> +static void\n> +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> +{\n> +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> +\tgst_atomic_queue_push(self->queue, (gpointer)buffer);\n> +}\n> +\n> +static void\n> +gst_libcamera_pool_init(GstLibcameraPool *self)\n> +{\n> +\tself->queue = gst_atomic_queue_new(4);\n> +}\n> +\n> +static void\n> +gst_libcamera_pool_finalize(GObject *object)\n> +{\n> +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(object);\n> +\tGstBuffer *buf;\n> +\n> +\twhile ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))\n> +\t\tgst_buffer_unref(buf);\n> +\n> +\tgst_atomic_queue_unref(self->queue);\n> +\tg_object_unref(self->allocator);\n> +\n> +\tG_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);\n> +}\n> +\n> +static void\n> +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)\n> +{\n> +\tauto *object_class = G_OBJECT_CLASS(klass);\n> +\tauto *pool_class = GST_BUFFER_POOL_CLASS(klass);\n> +\n> +\tobject_class->finalize = gst_libcamera_pool_finalize;\n> +\tpool_class->start = nullptr;\n> +\tpool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;\n> +\tpool_class->reset_buffer = gst_libcamera_pool_reset_buffer;\n> +\tpool_class->release_buffer = gst_libcamera_pool_release_buffer;\n> +}\n> +\n> +GstLibcameraPool *\n> +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)\n> +{\n> +\tauto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);\n> +\n> +\tpool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);\n> +\tpool->stream = stream;\n> +\n> +\tgsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);\n> +\tfor (gsize i = 0; i < pool_size; i++) {\n> +\t\tGstBuffer *buffer = gst_buffer_new();\n> +\t\tgst_atomic_queue_push(pool->queue, buffer);\n> +\t}\n> +\n> +\treturn pool;\n> +}\n> diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> new file mode 100644\n> index 0000000..ca6b299\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamerapool.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamerapool.h - GStreamer Buffer Pool\n> + *\n> + * This is a partial implementation of GstBufferPool intended for internal use\n> + * only. This pool cannot be configured or activated.\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \"gstlibcameraallocator.h\"\n> +\n> +#ifndef __GST_LIBCAMERA_POOL_H__\n> +#define __GST_LIBCAMERA_POOL_H__\n> +\n> +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()\n> +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,\n> +\t\t GST_LIBCAMERA, POOL, GstBufferPool)\n> +\n> +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> +\t\t\t\t\t libcamera::Stream *stream);\n> +\n> +#endif /* __GST_LIBCAMERA_POOL_H__ */\n> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> index e497bf4..346910f 100644\n> --- a/src/gstreamer/meson.build\n> +++ b/src/gstreamer/meson.build\n> @@ -4,6 +4,8 @@ libcamera_gst_sources = [\n> 'gstlibcamerasrc.cpp',\n> 'gstlibcameraprovider.cpp',\n> 'gstlibcamerapad.cpp',\n> + 'gstlibcameraallocator.cpp',\n> + 'gstlibcamerapool.cpp'\n> ]\n> \n> libcamera_gst_c_args = [\n> @@ -11,15 +13,18 @@ libcamera_gst_c_args = [\n> '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> ]\n> \n> -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> +gst_req = '>=1.16.1'\n\nMaybe gst_dep_version instead of gst_req ?\n\n> +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,\n> + required : get_option('gstreamer'))\n> +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,\n> required : get_option('gstreamer'))\n> \n> -if gst_dep.found()\n> +if gstvideo_dep.found() and gstallocator_dep.found()\n> libcamera_gst = shared_library('gstlibcamera',\n> libcamera_gst_sources,\n> c_args : libcamera_gst_c_args,\n> include_directories : [],\n> - dependencies : [libcamera_dep, gst_dep],\n> + dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],\n> install: true,\n> install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> )","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D12461020\n\tfor ;\n\tWed, 12 Feb 2020 01:16:40 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EDF639DA;\n\tWed, 12 Feb 2020 01:16:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581466600;\n\tbh=O274CxxWAMs9t0heWfHSK+wyEwyHEtRK8I9H9UTtTQg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XZ9YMX6YxmDepIoQpJnxsuArdQxTwhzArcguuO7Sludwsfwi9MLj0HUd25bqj1aBf\n\twlKWwm0NrK+zYM4IoNBoFf884RADGnR5zd9z9BcA+Z6raEpYXt7yV8kLQFFlbbqFmX\n\tb/1v8fbUomceMLa37HArWDnqz6jc9uVHRP00n2qs=","Date":"Wed, 12 Feb 2020 02:16:25 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200212001625.GQ20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-18-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-18-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Feb 2020 00:16:40 -0000"}},{"id":3694,"web_url":"https://patchwork.libcamera.org/comment/3694/","msgid":"","date":"2020-02-12T17:37:50","subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > This is needed to track the livetime of the FrameBufferAllocator in relation to\n> \n> s/livetime/lifetime/\n> \n> > the GstBuffer/GstMemory objects travelling inside GStreamer.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++\n> > src/gstreamer/gstlibcameraallocator.h | 29 +++\n> > src/gstreamer/gstlibcamerapool.cpp | 109 +++++++++++\n> > src/gstreamer/gstlibcamerapool.h | 27 +++\n> > src/gstreamer/meson.build | 11 +-\n> > 5 files changed, 413 insertions(+), 3 deletions(-)\n> > create mode 100644 src/gstreamer/gstlibcameraallocator.cpp\n> > create mode 100644 src/gstreamer/gstlibcameraallocator.h\n> > create mode 100644 src/gstreamer/gstlibcamerapool.cpp\n> > create mode 100644 src/gstreamer/gstlibcamerapool.h\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > new file mode 100644\n> > index 0000000..0f248b4\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -0,0 +1,240 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator\n> > + */\n> > +\n> > +#include \"gstlibcameraallocator.h\"\n> > +#include \"gstlibcamera-utils.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> \n> Could you please keep those headers alphabetically sorted ?\n> \n> > +\n> > +using namespace libcamera;\n> > +\n> > +/***********************************************************************/\n> > +/* Internal object for tracking memories associated with a FrameBuffer */\n> > +/***********************************************************************/\n> \n> Same comment as before regarding this, a doxygen-style comment with a\n> tiny bit of doc may be useful for inexperienced readers such as your\n> devoted reviewer :-) You could combine the next comment with it.\n\nOk, this was just a banner to help me visually locate the two objects\nin this .cpp file. I didn't know it was not allowed in this project. I\ncan add more information of course, I'm not familiar with doxygen doc,\nI would not make that part of the generated doc though, it's internal.\n\n> \n> > +\n> > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);\n> \n> It would be nice to avoid forward declarations, but it may not be worth\n> it.\n> \n> > +\n> > +/* This internal object is used to track the outstanding GstMemory object that\n> \n> s/object/objects/\n> \n> > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when\n> > + * al all outstranging GstMemory have returned.\n> \n> s/al all/all/ ?\n> s/outstranging/outstanding/ ?\n> \n> > + */\n> > +struct FrameWrap {\n> > +\tFrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > +\t\t gpointer stream);\n> > +\t~FrameWrap();\n> > +\n> > +\tvoid AcquirePlane() { ++outstanding_planes_; }\n> > +\tbool ReleasePlane() { return --outstanding_planes_ == 0; }\n> \n> Methods should start with a lowercase letter (camelCase, not CamelCase\n> as for type names).\n> \n> > +\n> > +\tgpointer stream_;\n> > +\tFrameBuffer *buffer_;\n> > +\tstd::vector planes_;\n> > +\tgint outstanding_planes_;\n> \n> outstandingPlanes_ as it's a C++ class ?\n> \n> > +};\n> > +\n> > +static GQuark\n> > +gst_libcamera_frame_quark(void)\n> \n> How about making this a static function of the FrameWrap class ?\n> \n> > +{\n> > +\tstatic gsize frame_quark = 0;\n> > +\n> > +\tif (g_once_init_enter(&frame_quark)) {\n> > +\t\tGQuark quark = g_quark_from_string(\"GstLibCameraFrameWrap\");\n> > +\t\tg_once_init_leave(&frame_quark, quark);\n> > +\t}\n> > +\n> > +\treturn frame_quark;\n> > +}\n> > +\n> > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > +\t\t gpointer stream)\n> > +\n> > +\t: stream_(stream),\n> > +\t buffer_(buffer),\n> > +\t outstanding_planes_(0)\n> > +{\n> > +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > +\t\tGstMemory *mem = gst_fd_allocator_alloc(allocator,\n> > +\t\t\t\t\t\t\tplane.fd.fd(),\n> > +\t\t\t\t\t\t\tplane.length,\n> > +\t\t\t\t\t\t\tGST_FD_MEMORY_FLAG_DONT_CLOSE);\n> > +\t\tgst_mini_object_set_qdata(GST_MINI_OBJECT(mem),\n> > +\t\t\t\t\t gst_libcamera_frame_quark(),\n> > +\t\t\t\t\t this, NULL);\n> > +\t\tGST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;\n> > +\t\tg_object_unref(mem->allocator);\n> > +\t\tplanes_.push_back(mem);\n> > +\t}\n> > +}\n> > +\n> > +FrameWrap::~FrameWrap()\n> > +{\n> > +\tfor (GstMemory *mem : planes_) {\n> > +\t\tGST_MINI_OBJECT(mem)->dispose = nullptr;\n> > +\t\tg_object_ref(mem->allocator);\n> > +\t\tgst_memory_unref(mem);\n> > +\t}\n> > +}\n> > +\n> > +/***********************************/\n> > +/* The GstAllocator implementation */\n> > +/***********************************/\n\nNeeded here to.\n\n> > +struct _GstLibcameraAllocator {\n> > +\tGstDmaBufAllocator parent;\n> > +\tFrameBufferAllocator *fb_allocator;\n> > +\t/* A hash table using Stream pointer as key and returning a GQueue of\n> > +\t * FrameWrap. */\n> > +\tGHashTable *pools;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > +\t GST_TYPE_DMABUF_ALLOCATOR);\n> > +\n> > +static gboolean\n> > +gst_libcamera_allocator_release(GstMiniObject *mini_object)\n> > +{\n> > +\tGstMemory *mem = GST_MEMORY_CAST(mini_object);\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\tgpointer data = gst_mini_object_get_qdata(mini_object,\n> > +\t\t\t\t\t\t gst_libcamera_frame_quark());\n> > +\tauto *frame = (FrameWrap *)data;\n> \n> Could you use reinterpret_cast(data) here ? And maybe\n> replace auto with FrameWrap as it's short.\n> \n> In general, please use static_cast or reinterpret_cast as appropriate,\n> as they offer more compile-time protection that C-style casts. I won't\n> comment on other occurrences.\n> \n> > +\n> > +\tgst_memory_ref(mem);\n> > +\n> > +\tif (frame->ReleasePlane()) {\n> > +\t\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);\n> > +\t\tg_return_val_if_fail(pool, TRUE);\n> > +\t\tg_queue_push_tail(pool, frame);\n> > +\t}\n> > +\n> > +\t/* Keep last in case we are holding on the last allocator ref */\n> > +\tg_object_unref(mem->allocator);\n> > +\n> > +\t/* Rreturns FALSE so that our mini object isn't freed */\n> \n> s/Rreturns/Return/\n> \n> > +\treturn FALSE;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_free_pool(gpointer data)\n> > +{\n> > +\tGQueue *queue = (GQueue *)data;\n> > +\tFrameWrap *frame;\n> > +\n> > +\twhile ((frame = (FrameWrap *)g_queue_pop_head(queue))) {\n> > +\t\tg_warn_if_fail(frame->outstanding_planes_ == 0);\n> > +\t\tdelete frame;\n> > +\t}\n> > +\n> > +\tg_queue_free(queue);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)\n> > +{\n> > +\tself->pools = g_hash_table_new_full(NULL, NULL, NULL,\n> > +\t\t\t\t\t gst_libcamera_allocator_free_pool);\n> > +\tGST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_dispose(GObject *object)\n> > +{\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > +\n> > +\tif (self->pools) {\n> > +\t\tg_hash_table_unref(self->pools);\n> > +\t\tself->pools = NULL;\n> > +\t}\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > +\n> > +\tdelete self->fb_allocator;\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)\n> > +{\n> > +\tauto *allocator_class = GST_ALLOCATOR_CLASS(klass);\n> > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > +\n> > +\tobject_class->dispose = gst_libcamera_allocator_dispose;\n> > +\tobject_class->finalize = gst_libcamera_allocator_finalize;\n> > +\tallocator_class->alloc = NULL;\n> > +}\n> > +\n> > +GstLibcameraAllocator *\n> > +gst_libcamera_allocator_new(std::shared_ptr camera)\n> > +{\n> > +\tauto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > +\t\t\t\t\t\t\t nullptr);\n> > +\n> > +\tself->fb_allocator = FrameBufferAllocator::create(camera);\n> > +\tfor (Stream *stream : camera->streams()) {\n> > +\t\tgint ret;\n> > +\n> > +\t\tret = self->fb_allocator->allocate(stream);\n> > +\t\tif (ret == 0)\n> > +\t\t\treturn nullptr;\n> > +\n> > +\t\tGQueue *pool = g_queue_new();\n> > +\t\tfor (const std::unique_ptr &buffer :\n> > +\t\t self->fb_allocator->buffers(stream)) {\n> > +\t\t\tauto *fb = new FrameWrap(GST_ALLOCATOR(self),\n> > +\t\t\t\t\t\t buffer.get(), stream);\n> > +\t\t\tg_queue_push_tail(pool, fb);\n> > +\t\t}\n> > +\n> > +\t\tg_hash_table_insert(self->pools, stream, pool);\n> > +\t}\n> > +\n> > +\treturn self;\n> > +}\n> > +\n> > +bool\n> > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > +\t\t\t\t Stream *stream, GstBuffer *buffer)\n> > +{\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\n> > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > +\tg_return_val_if_fail(pool, false);\n> > +\n> > +\tauto *frame = (FrameWrap *)g_queue_pop_head(pool);\n> > +\tif (!frame)\n> > +\t\treturn false;\n> > +\n> > +\tfor (GstMemory *mem : frame->planes_) {\n> > +\t\tframe->AcquirePlane();\n> > +\t\tgst_buffer_append_memory(buffer, mem);\n> > +\t\tg_object_ref(mem->allocator);\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +gsize\n> > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> > +\t\t\t\t Stream *stream)\n> > +{\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\n> > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > +\tg_return_val_if_fail(pool, false);\n> > +\n> > +\treturn pool->length;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> > new file mode 100644\n> > index 0000000..f2a0f58\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraallocator.h\n> > @@ -0,0 +1,29 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraallocator.h - GStreamer Custom Allocator\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__\n> > +#define __GST_LIBCAMERA_ALLOCATOR_H__\n> > +\n> > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > +\t\t GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)\n> > +\n> > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr camera);\n> > +\n> > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > +\t\t\t\t\t libcamera::Stream *stream,\n> > +\t\t\t\t\t GstBuffer *buffer);\n> > +\n> > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> > +\t\t\t\t\t libcamera::Stream *stream);\n> > +\n> > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */\n> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> > new file mode 100644\n> > index 0000000..f84d1d6\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerapool.cpp\n> > @@ -0,0 +1,109 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerapool.cpp - GStreamer Buffer Pool\n> > + */\n> > +\n> > +#include \"gstlibcamerapool.h\"\n> > +#include \"gstlibcamera-utils.h\"\n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +struct _GstLibcameraPool {\n> > +\tGstBufferPool parent;\n> > +\n> > +\tGstAtomicQueue *queue;\n> > +\tGstLibcameraAllocator *allocator;\n> > +\tStream *stream;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);\n> > +\n> > +static GstFlowReturn\n> > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,\n> > +\t\t\t\t GstBufferPoolAcquireParams *params)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > +\tGstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);\n> > +\tif (!buf)\n> > +\t\treturn GST_FLOW_ERROR;\n> > +\n> > +\tif (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))\n> > +\t\treturn GST_FLOW_ERROR;\n> > +\n> > +\t*buffer = buf;\n> > +\treturn GST_FLOW_OK;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > +{\n> > +\tGstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);\n> > +\n> > +\t/* Clears all the memories and only pool the GstBuffer objects */\n> > +\tgst_buffer_remove_all_memory(buffer);\n> > +\tklass->reset_buffer(pool, buffer);\n> > +\tGST_BUFFER_FLAGS(buffer) = 0;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > +\tgst_atomic_queue_push(self->queue, (gpointer)buffer);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_init(GstLibcameraPool *self)\n> > +{\n> > +\tself->queue = gst_atomic_queue_new(4);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(object);\n> > +\tGstBuffer *buf;\n> > +\n> > +\twhile ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))\n> > +\t\tgst_buffer_unref(buf);\n> > +\n> > +\tgst_atomic_queue_unref(self->queue);\n> > +\tg_object_unref(self->allocator);\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)\n> > +{\n> > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > +\tauto *pool_class = GST_BUFFER_POOL_CLASS(klass);\n> > +\n> > +\tobject_class->finalize = gst_libcamera_pool_finalize;\n> > +\tpool_class->start = nullptr;\n> > +\tpool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;\n> > +\tpool_class->reset_buffer = gst_libcamera_pool_reset_buffer;\n> > +\tpool_class->release_buffer = gst_libcamera_pool_release_buffer;\n> > +}\n> > +\n> > +GstLibcameraPool *\n> > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)\n> > +{\n> > +\tauto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);\n> > +\n> > +\tpool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);\n> > +\tpool->stream = stream;\n> > +\n> > +\tgsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);\n> > +\tfor (gsize i = 0; i < pool_size; i++) {\n> > +\t\tGstBuffer *buffer = gst_buffer_new();\n> > +\t\tgst_atomic_queue_push(pool->queue, buffer);\n> > +\t}\n> > +\n> > +\treturn pool;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> > new file mode 100644\n> > index 0000000..ca6b299\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerapool.h\n> > @@ -0,0 +1,27 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerapool.h - GStreamer Buffer Pool\n> > + *\n> > + * This is a partial implementation of GstBufferPool intended for internal use\n> > + * only. This pool cannot be configured or activated.\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"gstlibcameraallocator.h\"\n> > +\n> > +#ifndef __GST_LIBCAMERA_POOL_H__\n> > +#define __GST_LIBCAMERA_POOL_H__\n> > +\n> > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,\n> > +\t\t GST_LIBCAMERA, POOL, GstBufferPool)\n> > +\n> > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> > +\t\t\t\t\t libcamera::Stream *stream);\n> > +\n> > +#endif /* __GST_LIBCAMERA_POOL_H__ */\n> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > index e497bf4..346910f 100644\n> > --- a/src/gstreamer/meson.build\n> > +++ b/src/gstreamer/meson.build\n> > @@ -4,6 +4,8 @@ libcamera_gst_sources = [\n> > 'gstlibcamerasrc.cpp',\n> > 'gstlibcameraprovider.cpp',\n> > 'gstlibcamerapad.cpp',\n> > + 'gstlibcameraallocator.cpp',\n> > + 'gstlibcamerapool.cpp'\n> > ]\n> > \n> > libcamera_gst_c_args = [\n> > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [\n> > '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > ]\n> > \n> > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > +gst_req = '>=1.16.1'\n> \n> Maybe gst_dep_version instead of gst_req ?\n> \n> > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,\n> > + required : get_option('gstreamer'))\n> > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,\n> > required : get_option('gstreamer'))\n> > \n> > -if gst_dep.found()\n> > +if gstvideo_dep.found() and gstallocator_dep.found()\n> > libcamera_gst = shared_library('gstlibcamera',\n> > libcamera_gst_sources,\n> > c_args : libcamera_gst_c_args,\n> > include_directories : [],\n> > - dependencies : [libcamera_dep, gst_dep],\n> > + dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],\n> > install: true,\n> > install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > )","headers":{"Return-Path":"","Received":["from mail-qt1-x82a.google.com (mail-qt1-x82a.google.com\n\t[IPv6:2607:f8b0:4864:20::82a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD77B6043B\n\tfor ;\n\tWed, 12 Feb 2020 18:37:52 +0100 (CET)","by mail-qt1-x82a.google.com with SMTP id l21so2190431qtr.8\n\tfor ;\n\tWed, 12 Feb 2020 09:37:52 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\ti5sm559330qtq.12.2020.02.12.09.37.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 12 Feb 2020 09:37:51 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=uvFvWVzdQfCX5xhGQ4mMA98qE4vOC6JQ1yqU2jyYSyY=;\n\tb=XzVc6U5pJKlBArzYURWs1HFsbWysVcv1F+g4DxnCIXEjGKehckN+wWzEnlGULLddF6\n\ttTQyumxLasH+JGNZ6s4tWhmkQ/e0LPsXOG7lP4TKZmTvxcbmSaWQwX8DIx6ctonQiKu+\n\thxys3iUSP0ERE42neoZsahAEomOgqErscaNRBgsYe/hbVu578B0FNYrZ86GHNOi4yv+5\n\tw61jMLZjmRXTQSIZNn8J/+aSlDmQ2Q0hBSLbEOymgwOAK/UJITLbu/BDimy6VXdpzP0p\n\to1CvLIAocW8881WBAGolGyQOQ0hS45J+F5qQBkrLHU2s+dLEJ4HTdOkHoUmjA91mX0cU\n\t6/3w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=uvFvWVzdQfCX5xhGQ4mMA98qE4vOC6JQ1yqU2jyYSyY=;\n\tb=gpIyzoE7NlxFhH9jmvV/FExuzhN5L/jjhdo3CXknrNONisrXEjriBA09bWm0V+UQZh\n\teAMQ3LP4lw9SBToz2rzdcr9nSoRlGDZQQmWbTvHsMU8IQsho+kBYbEakZtMDUNlOGs10\n\tZ08U69WjwwAfYxH7G+sL9KqgzZJprJhBW6HIEipUqoi7PJ6bQo2XmV76NBbzMOMJ0bEj\n\tN/C+HrMccuzo4x3UTeIwsYE+6rlbPD2YxrMhJw8/E2u9w6nzUBkFwPmzxOnwkoRreDp2\n\tfsxHgcBjFY07h+HcrWwuMbb8f7JI0Evs5l35fWG132pxkXx6sWfrCYRhgWY8ycQ7yzAi\n\tkwsw==","X-Gm-Message-State":"APjAAAUQZfiQ/nQGa58NkJmvnz7CFlLxd8xVtk5UMl58EO033kkuvyXS\n\t/Mbz/HOQDA+/7yn3dg65WVYi7kREeOCi3A==","X-Google-Smtp-Source":"APXvYqyQp7ZqP/+3JGpnQQC1X8ARM5fKODiRj+lKSAtLaTd4hy3tWArWjzXai8ZzcwbZFe7GEI32cg==","X-Received":"by 2002:ac8:10d:: with SMTP id e13mr8034467qtg.294.1581529071684;\n\tWed, 12 Feb 2020 09:37:51 -0800 (PST)","Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Wed, 12 Feb 2020 12:37:50 -0500","In-Reply-To":"<20200212001625.GQ20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-18-nicolas@ndufresne.ca>\n\t<20200212001625.GQ20823@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Feb 2020 17:37:53 -0000"}},{"id":3699,"web_url":"https://patchwork.libcamera.org/comment/3699/","msgid":"<20200212201002.GB17626@pendragon.ideasonboard.com>","date":"2020-02-12T20:10:02","subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Wed, Feb 12, 2020 at 12:37:50PM -0500, Nicolas Dufresne wrote:\n> Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :\n> > On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne \n> > > \n> > > This is needed to track the livetime of the FrameBufferAllocator in relation to\n> > \n> > s/livetime/lifetime/\n> > \n> > > the GstBuffer/GstMemory objects travelling inside GStreamer.\n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++\n> > > src/gstreamer/gstlibcameraallocator.h | 29 +++\n> > > src/gstreamer/gstlibcamerapool.cpp | 109 +++++++++++\n> > > src/gstreamer/gstlibcamerapool.h | 27 +++\n> > > src/gstreamer/meson.build | 11 +-\n> > > 5 files changed, 413 insertions(+), 3 deletions(-)\n> > > create mode 100644 src/gstreamer/gstlibcameraallocator.cpp\n> > > create mode 100644 src/gstreamer/gstlibcameraallocator.h\n> > > create mode 100644 src/gstreamer/gstlibcamerapool.cpp\n> > > create mode 100644 src/gstreamer/gstlibcamerapool.h\n> > > \n> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > > new file mode 100644\n> > > index 0000000..0f248b4\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > > @@ -0,0 +1,240 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator\n> > > + */\n> > > +\n> > > +#include \"gstlibcameraallocator.h\"\n> > > +#include \"gstlibcamera-utils.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > \n> > Could you please keep those headers alphabetically sorted ?\n> > \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +/***********************************************************************/\n> > > +/* Internal object for tracking memories associated with a FrameBuffer */\n> > > +/***********************************************************************/\n> > \n> > Same comment as before regarding this, a doxygen-style comment with a\n> > tiny bit of doc may be useful for inexperienced readers such as your\n> > devoted reviewer :-) You could combine the next comment with it.\n> \n> Ok, this was just a banner to help me visually locate the two objects\n> in this .cpp file. I didn't know it was not allowed in this project. I\n> can add more information of course, I'm not familiar with doxygen doc,\n> I would not make that part of the generated doc though, it's internal.\n\nI wouldn't go as far as saying it's not allowed :-) I only wanted to\npoint out that a comment along the lines of\n\n/**\n * \\struct FrameWrap\n * \\brief Track memories associated with a FrameBuffer\n *\n * This structure .....\n * .....\n * .....\n */\n\ncould provide a bit of useful information to the reader and serve as a\nvisual separator. Please pick the style you like the most.\n\n> > > +\n> > > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);\n> > \n> > It would be nice to avoid forward declarations, but it may not be worth\n> > it.\n> > \n> > > +\n> > > +/* This internal object is used to track the outstanding GstMemory object that\n> > \n> > s/object/objects/\n> > \n> > > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when\n> > > + * al all outstranging GstMemory have returned.\n> > \n> > s/al all/all/ ?\n> > s/outstranging/outstanding/ ?\n> > \n> > > + */\n> > > +struct FrameWrap {\n> > > +\tFrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > > +\t\t gpointer stream);\n> > > +\t~FrameWrap();\n> > > +\n> > > +\tvoid AcquirePlane() { ++outstanding_planes_; }\n> > > +\tbool ReleasePlane() { return --outstanding_planes_ == 0; }\n> > \n> > Methods should start with a lowercase letter (camelCase, not CamelCase\n> > as for type names).\n> > \n> > > +\n> > > +\tgpointer stream_;\n> > > +\tFrameBuffer *buffer_;\n> > > +\tstd::vector planes_;\n> > > +\tgint outstanding_planes_;\n> > \n> > outstandingPlanes_ as it's a C++ class ?\n> > \n> > > +};\n> > > +\n> > > +static GQuark\n> > > +gst_libcamera_frame_quark(void)\n> > \n> > How about making this a static function of the FrameWrap class ?\n> > \n> > > +{\n> > > +\tstatic gsize frame_quark = 0;\n> > > +\n> > > +\tif (g_once_init_enter(&frame_quark)) {\n> > > +\t\tGQuark quark = g_quark_from_string(\"GstLibCameraFrameWrap\");\n> > > +\t\tg_once_init_leave(&frame_quark, quark);\n> > > +\t}\n> > > +\n> > > +\treturn frame_quark;\n> > > +}\n> > > +\n> > > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > > +\t\t gpointer stream)\n> > > +\n> > > +\t: stream_(stream),\n> > > +\t buffer_(buffer),\n> > > +\t outstanding_planes_(0)\n> > > +{\n> > > +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > +\t\tGstMemory *mem = gst_fd_allocator_alloc(allocator,\n> > > +\t\t\t\t\t\t\tplane.fd.fd(),\n> > > +\t\t\t\t\t\t\tplane.length,\n> > > +\t\t\t\t\t\t\tGST_FD_MEMORY_FLAG_DONT_CLOSE);\n> > > +\t\tgst_mini_object_set_qdata(GST_MINI_OBJECT(mem),\n> > > +\t\t\t\t\t gst_libcamera_frame_quark(),\n> > > +\t\t\t\t\t this, NULL);\n> > > +\t\tGST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;\n> > > +\t\tg_object_unref(mem->allocator);\n> > > +\t\tplanes_.push_back(mem);\n> > > +\t}\n> > > +}\n> > > +\n> > > +FrameWrap::~FrameWrap()\n> > > +{\n> > > +\tfor (GstMemory *mem : planes_) {\n> > > +\t\tGST_MINI_OBJECT(mem)->dispose = nullptr;\n> > > +\t\tg_object_ref(mem->allocator);\n> > > +\t\tgst_memory_unref(mem);\n> > > +\t}\n> > > +}\n> > > +\n> > > +/***********************************/\n> > > +/* The GstAllocator implementation */\n> > > +/***********************************/\n> \n> Needed here to.\n> \n> > > +struct _GstLibcameraAllocator {\n> > > +\tGstDmaBufAllocator parent;\n> > > +\tFrameBufferAllocator *fb_allocator;\n> > > +\t/* A hash table using Stream pointer as key and returning a GQueue of\n> > > +\t * FrameWrap. */\n> > > +\tGHashTable *pools;\n> > > +};\n> > > +\n> > > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > > +\t GST_TYPE_DMABUF_ALLOCATOR);\n> > > +\n> > > +static gboolean\n> > > +gst_libcamera_allocator_release(GstMiniObject *mini_object)\n> > > +{\n> > > +\tGstMemory *mem = GST_MEMORY_CAST(mini_object);\n> > > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);\n> > > +\tGST_OBJECT_LOCKER(self);\n> > > +\tgpointer data = gst_mini_object_get_qdata(mini_object,\n> > > +\t\t\t\t\t\t gst_libcamera_frame_quark());\n> > > +\tauto *frame = (FrameWrap *)data;\n> > \n> > Could you use reinterpret_cast(data) here ? And maybe\n> > replace auto with FrameWrap as it's short.\n> > \n> > In general, please use static_cast or reinterpret_cast as appropriate,\n> > as they offer more compile-time protection that C-style casts. I won't\n> > comment on other occurrences.\n> > \n> > > +\n> > > +\tgst_memory_ref(mem);\n> > > +\n> > > +\tif (frame->ReleasePlane()) {\n> > > +\t\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);\n> > > +\t\tg_return_val_if_fail(pool, TRUE);\n> > > +\t\tg_queue_push_tail(pool, frame);\n> > > +\t}\n> > > +\n> > > +\t/* Keep last in case we are holding on the last allocator ref */\n> > > +\tg_object_unref(mem->allocator);\n> > > +\n> > > +\t/* Rreturns FALSE so that our mini object isn't freed */\n> > \n> > s/Rreturns/Return/\n> > \n> > > +\treturn FALSE;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_allocator_free_pool(gpointer data)\n> > > +{\n> > > +\tGQueue *queue = (GQueue *)data;\n> > > +\tFrameWrap *frame;\n> > > +\n> > > +\twhile ((frame = (FrameWrap *)g_queue_pop_head(queue))) {\n> > > +\t\tg_warn_if_fail(frame->outstanding_planes_ == 0);\n> > > +\t\tdelete frame;\n> > > +\t}\n> > > +\n> > > +\tg_queue_free(queue);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)\n> > > +{\n> > > +\tself->pools = g_hash_table_new_full(NULL, NULL, NULL,\n> > > +\t\t\t\t\t gst_libcamera_allocator_free_pool);\n> > > +\tGST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_allocator_dispose(GObject *object)\n> > > +{\n> > > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > > +\n> > > +\tif (self->pools) {\n> > > +\t\tg_hash_table_unref(self->pools);\n> > > +\t\tself->pools = NULL;\n> > > +\t}\n> > > +\n> > > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_allocator_finalize(GObject *object)\n> > > +{\n> > > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > > +\n> > > +\tdelete self->fb_allocator;\n> > > +\n> > > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)\n> > > +{\n> > > +\tauto *allocator_class = GST_ALLOCATOR_CLASS(klass);\n> > > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > > +\n> > > +\tobject_class->dispose = gst_libcamera_allocator_dispose;\n> > > +\tobject_class->finalize = gst_libcamera_allocator_finalize;\n> > > +\tallocator_class->alloc = NULL;\n> > > +}\n> > > +\n> > > +GstLibcameraAllocator *\n> > > +gst_libcamera_allocator_new(std::shared_ptr camera)\n> > > +{\n> > > +\tauto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > > +\t\t\t\t\t\t\t nullptr);\n> > > +\n> > > +\tself->fb_allocator = FrameBufferAllocator::create(camera);\n> > > +\tfor (Stream *stream : camera->streams()) {\n> > > +\t\tgint ret;\n> > > +\n> > > +\t\tret = self->fb_allocator->allocate(stream);\n> > > +\t\tif (ret == 0)\n> > > +\t\t\treturn nullptr;\n> > > +\n> > > +\t\tGQueue *pool = g_queue_new();\n> > > +\t\tfor (const std::unique_ptr &buffer :\n> > > +\t\t self->fb_allocator->buffers(stream)) {\n> > > +\t\t\tauto *fb = new FrameWrap(GST_ALLOCATOR(self),\n> > > +\t\t\t\t\t\t buffer.get(), stream);\n> > > +\t\t\tg_queue_push_tail(pool, fb);\n> > > +\t\t}\n> > > +\n> > > +\t\tg_hash_table_insert(self->pools, stream, pool);\n> > > +\t}\n> > > +\n> > > +\treturn self;\n> > > +}\n> > > +\n> > > +bool\n> > > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > > +\t\t\t\t Stream *stream, GstBuffer *buffer)\n> > > +{\n> > > +\tGST_OBJECT_LOCKER(self);\n> > > +\n> > > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > > +\tg_return_val_if_fail(pool, false);\n> > > +\n> > > +\tauto *frame = (FrameWrap *)g_queue_pop_head(pool);\n> > > +\tif (!frame)\n> > > +\t\treturn false;\n> > > +\n> > > +\tfor (GstMemory *mem : frame->planes_) {\n> > > +\t\tframe->AcquirePlane();\n> > > +\t\tgst_buffer_append_memory(buffer, mem);\n> > > +\t\tg_object_ref(mem->allocator);\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +gsize\n> > > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> > > +\t\t\t\t Stream *stream)\n> > > +{\n> > > +\tGST_OBJECT_LOCKER(self);\n> > > +\n> > > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > > +\tg_return_val_if_fail(pool, false);\n> > > +\n> > > +\treturn pool->length;\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> > > new file mode 100644\n> > > index 0000000..f2a0f58\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcameraallocator.h\n> > > @@ -0,0 +1,29 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcameraallocator.h - GStreamer Custom Allocator\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__\n> > > +#define __GST_LIBCAMERA_ALLOCATOR_H__\n> > > +\n> > > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()\n> > > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > > +\t\t GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)\n> > > +\n> > > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr camera);\n> > > +\n> > > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > > +\t\t\t\t\t libcamera::Stream *stream,\n> > > +\t\t\t\t\t GstBuffer *buffer);\n> > > +\n> > > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> > > +\t\t\t\t\t libcamera::Stream *stream);\n> > > +\n> > > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */\n> > > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> > > new file mode 100644\n> > > index 0000000..f84d1d6\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamerapool.cpp\n> > > @@ -0,0 +1,109 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamerapool.cpp - GStreamer Buffer Pool\n> > > + */\n> > > +\n> > > +#include \"gstlibcamerapool.h\"\n> > > +#include \"gstlibcamera-utils.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +struct _GstLibcameraPool {\n> > > +\tGstBufferPool parent;\n> > > +\n> > > +\tGstAtomicQueue *queue;\n> > > +\tGstLibcameraAllocator *allocator;\n> > > +\tStream *stream;\n> > > +};\n> > > +\n> > > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);\n> > > +\n> > > +static GstFlowReturn\n> > > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,\n> > > +\t\t\t\t GstBufferPoolAcquireParams *params)\n> > > +{\n> > > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > > +\tGstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);\n> > > +\tif (!buf)\n> > > +\t\treturn GST_FLOW_ERROR;\n> > > +\n> > > +\tif (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))\n> > > +\t\treturn GST_FLOW_ERROR;\n> > > +\n> > > +\t*buffer = buf;\n> > > +\treturn GST_FLOW_OK;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > > +{\n> > > +\tGstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);\n> > > +\n> > > +\t/* Clears all the memories and only pool the GstBuffer objects */\n> > > +\tgst_buffer_remove_all_memory(buffer);\n> > > +\tklass->reset_buffer(pool, buffer);\n> > > +\tGST_BUFFER_FLAGS(buffer) = 0;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > > +{\n> > > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > > +\tgst_atomic_queue_push(self->queue, (gpointer)buffer);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_pool_init(GstLibcameraPool *self)\n> > > +{\n> > > +\tself->queue = gst_atomic_queue_new(4);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_pool_finalize(GObject *object)\n> > > +{\n> > > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(object);\n> > > +\tGstBuffer *buf;\n> > > +\n> > > +\twhile ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))\n> > > +\t\tgst_buffer_unref(buf);\n> > > +\n> > > +\tgst_atomic_queue_unref(self->queue);\n> > > +\tg_object_unref(self->allocator);\n> > > +\n> > > +\tG_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)\n> > > +{\n> > > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > > +\tauto *pool_class = GST_BUFFER_POOL_CLASS(klass);\n> > > +\n> > > +\tobject_class->finalize = gst_libcamera_pool_finalize;\n> > > +\tpool_class->start = nullptr;\n> > > +\tpool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;\n> > > +\tpool_class->reset_buffer = gst_libcamera_pool_reset_buffer;\n> > > +\tpool_class->release_buffer = gst_libcamera_pool_release_buffer;\n> > > +}\n> > > +\n> > > +GstLibcameraPool *\n> > > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)\n> > > +{\n> > > +\tauto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);\n> > > +\n> > > +\tpool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);\n> > > +\tpool->stream = stream;\n> > > +\n> > > +\tgsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);\n> > > +\tfor (gsize i = 0; i < pool_size; i++) {\n> > > +\t\tGstBuffer *buffer = gst_buffer_new();\n> > > +\t\tgst_atomic_queue_push(pool->queue, buffer);\n> > > +\t}\n> > > +\n> > > +\treturn pool;\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> > > new file mode 100644\n> > > index 0000000..ca6b299\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamerapool.h\n> > > @@ -0,0 +1,27 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamerapool.h - GStreamer Buffer Pool\n> > > + *\n> > > + * This is a partial implementation of GstBufferPool intended for internal use\n> > > + * only. This pool cannot be configured or activated.\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"gstlibcameraallocator.h\"\n> > > +\n> > > +#ifndef __GST_LIBCAMERA_POOL_H__\n> > > +#define __GST_LIBCAMERA_POOL_H__\n> > > +\n> > > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()\n> > > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,\n> > > +\t\t GST_LIBCAMERA, POOL, GstBufferPool)\n> > > +\n> > > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> > > +\t\t\t\t\t libcamera::Stream *stream);\n> > > +\n> > > +#endif /* __GST_LIBCAMERA_POOL_H__ */\n> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > index e497bf4..346910f 100644\n> > > --- a/src/gstreamer/meson.build\n> > > +++ b/src/gstreamer/meson.build\n> > > @@ -4,6 +4,8 @@ libcamera_gst_sources = [\n> > > 'gstlibcamerasrc.cpp',\n> > > 'gstlibcameraprovider.cpp',\n> > > 'gstlibcamerapad.cpp',\n> > > + 'gstlibcameraallocator.cpp',\n> > > + 'gstlibcamerapool.cpp'\n> > > ]\n> > > \n> > > libcamera_gst_c_args = [\n> > > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [\n> > > '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > > ]\n> > > \n> > > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > > +gst_req = '>=1.16.1'\n> > \n> > Maybe gst_dep_version instead of gst_req ?\n> > \n> > > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,\n> > > + required : get_option('gstreamer'))\n> > > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,\n> > > required : get_option('gstreamer'))\n> > > \n> > > -if gst_dep.found()\n> > > +if gstvideo_dep.found() and gstallocator_dep.found()\n> > > libcamera_gst = shared_library('gstlibcamera',\n> > > libcamera_gst_sources,\n> > > c_args : libcamera_gst_c_args,\n> > > include_directories : [],\n> > > - dependencies : [libcamera_dep, gst_dep],\n> > > + dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],\n> > > install: true,\n> > > install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > > )\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80C8861917\n\tfor ;\n\tWed, 12 Feb 2020 21:10:19 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E5B30808;\n\tWed, 12 Feb 2020 21:10:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581538219;\n\tbh=FCcF9zDP1Q3L4yLirUC2f1ut3pDxCGTVmVRIPvFz7xw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o/tm+l0RJf2EzBRLnxGLQu3Kh78wi1vzkERDXabvAL+h/vTDLYHfMLE7ym1fhV1fv\n\tWOeBGXUKIQhYEcoQJwe/Rrj42gIyGYKR406HPcV53QozrpHmfD2Bmn5adWY+bbi6pc\n\tD59ibpiIi8Fl86KWm6IuNnZCj5duw2QxENca+sOU=","Date":"Wed, 12 Feb 2020 22:10:02 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200212201002.GB17626@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-18-nicolas@ndufresne.ca>\n\t<20200212001625.GQ20823@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Feb 2020 20:10:19 -0000"}},{"id":3846,"web_url":"https://patchwork.libcamera.org/comment/3846/","msgid":"","date":"2020-02-25T20:15:17","subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > This is needed to track the livetime of the FrameBufferAllocator in relation to\n> \n> s/livetime/lifetime/\n> \n> > the GstBuffer/GstMemory objects travelling inside GStreamer.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++\n> > src/gstreamer/gstlibcameraallocator.h | 29 +++\n> > src/gstreamer/gstlibcamerapool.cpp | 109 +++++++++++\n> > src/gstreamer/gstlibcamerapool.h | 27 +++\n> > src/gstreamer/meson.build | 11 +-\n> > 5 files changed, 413 insertions(+), 3 deletions(-)\n> > create mode 100644 src/gstreamer/gstlibcameraallocator.cpp\n> > create mode 100644 src/gstreamer/gstlibcameraallocator.h\n> > create mode 100644 src/gstreamer/gstlibcamerapool.cpp\n> > create mode 100644 src/gstreamer/gstlibcamerapool.h\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > new file mode 100644\n> > index 0000000..0f248b4\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -0,0 +1,240 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator\n> > + */\n> > +\n> > +#include \"gstlibcameraallocator.h\"\n> > +#include \"gstlibcamera-utils.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> \n> Could you please keep those headers alphabetically sorted ?\n> \n> > +\n> > +using namespace libcamera;\n> > +\n> > +/***********************************************************************/\n> > +/* Internal object for tracking memories associated with a FrameBuffer */\n> > +/***********************************************************************/\n> \n> Same comment as before regarding this, a doxygen-style comment with a\n> tiny bit of doc may be useful for inexperienced readers such as your\n> devoted reviewer :-) You could combine the next comment with it.\n> \n> > +\n> > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);\n> \n> It would be nice to avoid forward declarations, but it may not be worth\n> it.\n\nIt's always my goal, but this one was not avoidable.\n\n> \n> > +\n> > +/* This internal object is used to track the outstanding GstMemory object that\n> \n> s/object/objects/\n> \n> > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when\n> > + * al all outstranging GstMemory have returned.\n> \n> s/al all/all/ ?\n> s/outstranging/outstanding/ ?\n> \n> > + */\n> > +struct FrameWrap {\n> > +\tFrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > +\t\t gpointer stream);\n> > +\t~FrameWrap();\n> > +\n> > +\tvoid AcquirePlane() { ++outstanding_planes_; }\n> > +\tbool ReleasePlane() { return --outstanding_planes_ == 0; }\n> \n> Methods should start with a lowercase letter (camelCase, not CamelCase\n> as for type names).\n> \n> > +\n> > +\tgpointer stream_;\n> > +\tFrameBuffer *buffer_;\n> > +\tstd::vector planes_;\n> > +\tgint outstanding_planes_;\n> \n> outstandingPlanes_ as it's a C++ class ?\n> \n> > +};\n> > +\n> > +static GQuark\n> > +gst_libcamera_frame_quark(void)\n> \n> How about making this a static function of the FrameWrap class ?\n> \n> > +{\n> > +\tstatic gsize frame_quark = 0;\n> > +\n> > +\tif (g_once_init_enter(&frame_quark)) {\n> > +\t\tGQuark quark = g_quark_from_string(\"GstLibCameraFrameWrap\");\n> > +\t\tg_once_init_leave(&frame_quark, quark);\n> > +\t}\n> > +\n> > +\treturn frame_quark;\n> > +}\n> > +\n> > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,\n> > +\t\t gpointer stream)\n> > +\n> > +\t: stream_(stream),\n> > +\t buffer_(buffer),\n> > +\t outstanding_planes_(0)\n> > +{\n> > +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > +\t\tGstMemory *mem = gst_fd_allocator_alloc(allocator,\n> > +\t\t\t\t\t\t\tplane.fd.fd(),\n> > +\t\t\t\t\t\t\tplane.length,\n> > +\t\t\t\t\t\t\tGST_FD_MEMORY_FLAG_DONT_CLOSE);\n> > +\t\tgst_mini_object_set_qdata(GST_MINI_OBJECT(mem),\n> > +\t\t\t\t\t gst_libcamera_frame_quark(),\n> > +\t\t\t\t\t this, NULL);\n> > +\t\tGST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;\n> > +\t\tg_object_unref(mem->allocator);\n> > +\t\tplanes_.push_back(mem);\n> > +\t}\n> > +}\n> > +\n> > +FrameWrap::~FrameWrap()\n> > +{\n> > +\tfor (GstMemory *mem : planes_) {\n> > +\t\tGST_MINI_OBJECT(mem)->dispose = nullptr;\n> > +\t\tg_object_ref(mem->allocator);\n> > +\t\tgst_memory_unref(mem);\n> > +\t}\n> > +}\n> > +\n> > +/***********************************/\n> > +/* The GstAllocator implementation */\n> > +/***********************************/\n> > +struct _GstLibcameraAllocator {\n> > +\tGstDmaBufAllocator parent;\n> > +\tFrameBufferAllocator *fb_allocator;\n> > +\t/* A hash table using Stream pointer as key and returning a GQueue of\n> > +\t * FrameWrap. */\n> > +\tGHashTable *pools;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > +\t GST_TYPE_DMABUF_ALLOCATOR);\n> > +\n> > +static gboolean\n> > +gst_libcamera_allocator_release(GstMiniObject *mini_object)\n> > +{\n> > +\tGstMemory *mem = GST_MEMORY_CAST(mini_object);\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\tgpointer data = gst_mini_object_get_qdata(mini_object,\n> > +\t\t\t\t\t\t gst_libcamera_frame_quark());\n> > +\tauto *frame = (FrameWrap *)data;\n> \n> Could you use reinterpret_cast(data) here ? And maybe\n> replace auto with FrameWrap as it's short.\n> \n> In general, please use static_cast or reinterpret_cast as appropriate,\n> as they offer more compile-time protection that C-style casts. I won't\n> comment on other occurrences.\n> \n> > +\n> > +\tgst_memory_ref(mem);\n> > +\n> > +\tif (frame->ReleasePlane()) {\n> > +\t\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);\n> > +\t\tg_return_val_if_fail(pool, TRUE);\n> > +\t\tg_queue_push_tail(pool, frame);\n> > +\t}\n> > +\n> > +\t/* Keep last in case we are holding on the last allocator ref */\n> > +\tg_object_unref(mem->allocator);\n> > +\n> > +\t/* Rreturns FALSE so that our mini object isn't freed */\n> \n> s/Rreturns/Return/\n> \n> > +\treturn FALSE;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_free_pool(gpointer data)\n> > +{\n> > +\tGQueue *queue = (GQueue *)data;\n> > +\tFrameWrap *frame;\n> > +\n> > +\twhile ((frame = (FrameWrap *)g_queue_pop_head(queue))) {\n> > +\t\tg_warn_if_fail(frame->outstanding_planes_ == 0);\n> > +\t\tdelete frame;\n> > +\t}\n> > +\n> > +\tg_queue_free(queue);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)\n> > +{\n> > +\tself->pools = g_hash_table_new_full(NULL, NULL, NULL,\n> > +\t\t\t\t\t gst_libcamera_allocator_free_pool);\n> > +\tGST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_dispose(GObject *object)\n> > +{\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > +\n> > +\tif (self->pools) {\n> > +\t\tg_hash_table_unref(self->pools);\n> > +\t\tself->pools = NULL;\n> > +\t}\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);\n> > +\n> > +\tdelete self->fb_allocator;\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)\n> > +{\n> > +\tauto *allocator_class = GST_ALLOCATOR_CLASS(klass);\n> > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > +\n> > +\tobject_class->dispose = gst_libcamera_allocator_dispose;\n> > +\tobject_class->finalize = gst_libcamera_allocator_finalize;\n> > +\tallocator_class->alloc = NULL;\n> > +}\n> > +\n> > +GstLibcameraAllocator *\n> > +gst_libcamera_allocator_new(std::shared_ptr camera)\n> > +{\n> > +\tauto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > +\t\t\t\t\t\t\t nullptr);\n> > +\n> > +\tself->fb_allocator = FrameBufferAllocator::create(camera);\n> > +\tfor (Stream *stream : camera->streams()) {\n> > +\t\tgint ret;\n> > +\n> > +\t\tret = self->fb_allocator->allocate(stream);\n> > +\t\tif (ret == 0)\n> > +\t\t\treturn nullptr;\n> > +\n> > +\t\tGQueue *pool = g_queue_new();\n> > +\t\tfor (const std::unique_ptr &buffer :\n> > +\t\t self->fb_allocator->buffers(stream)) {\n> > +\t\t\tauto *fb = new FrameWrap(GST_ALLOCATOR(self),\n> > +\t\t\t\t\t\t buffer.get(), stream);\n> > +\t\t\tg_queue_push_tail(pool, fb);\n> > +\t\t}\n> > +\n> > +\t\tg_hash_table_insert(self->pools, stream, pool);\n> > +\t}\n> > +\n> > +\treturn self;\n> > +}\n> > +\n> > +bool\n> > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > +\t\t\t\t Stream *stream, GstBuffer *buffer)\n> > +{\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\n> > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > +\tg_return_val_if_fail(pool, false);\n> > +\n> > +\tauto *frame = (FrameWrap *)g_queue_pop_head(pool);\n> > +\tif (!frame)\n> > +\t\treturn false;\n> > +\n> > +\tfor (GstMemory *mem : frame->planes_) {\n> > +\t\tframe->AcquirePlane();\n> > +\t\tgst_buffer_append_memory(buffer, mem);\n> > +\t\tg_object_ref(mem->allocator);\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +gsize\n> > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> > +\t\t\t\t Stream *stream)\n> > +{\n> > +\tGST_OBJECT_LOCKER(self);\n> > +\n> > +\tauto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);\n> > +\tg_return_val_if_fail(pool, false);\n> > +\n> > +\treturn pool->length;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> > new file mode 100644\n> > index 0000000..f2a0f58\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraallocator.h\n> > @@ -0,0 +1,29 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraallocator.h - GStreamer Custom Allocator\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__\n> > +#define __GST_LIBCAMERA_ALLOCATOR_H__\n> > +\n> > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > +\t\t GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)\n> > +\n> > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr camera);\n> > +\n> > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > +\t\t\t\t\t libcamera::Stream *stream,\n> > +\t\t\t\t\t GstBuffer *buffer);\n> > +\n> > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> > +\t\t\t\t\t libcamera::Stream *stream);\n> > +\n> > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */\n> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> > new file mode 100644\n> > index 0000000..f84d1d6\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerapool.cpp\n> > @@ -0,0 +1,109 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerapool.cpp - GStreamer Buffer Pool\n> > + */\n> > +\n> > +#include \"gstlibcamerapool.h\"\n> > +#include \"gstlibcamera-utils.h\"\n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +struct _GstLibcameraPool {\n> > +\tGstBufferPool parent;\n> > +\n> > +\tGstAtomicQueue *queue;\n> > +\tGstLibcameraAllocator *allocator;\n> > +\tStream *stream;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);\n> > +\n> > +static GstFlowReturn\n> > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,\n> > +\t\t\t\t GstBufferPoolAcquireParams *params)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > +\tGstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);\n> > +\tif (!buf)\n> > +\t\treturn GST_FLOW_ERROR;\n> > +\n> > +\tif (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))\n> > +\t\treturn GST_FLOW_ERROR;\n> > +\n> > +\t*buffer = buf;\n> > +\treturn GST_FLOW_OK;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > +{\n> > +\tGstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);\n> > +\n> > +\t/* Clears all the memories and only pool the GstBuffer objects */\n> > +\tgst_buffer_remove_all_memory(buffer);\n> > +\tklass->reset_buffer(pool, buffer);\n> > +\tGST_BUFFER_FLAGS(buffer) = 0;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);\n> > +\tgst_atomic_queue_push(self->queue, (gpointer)buffer);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_init(GstLibcameraPool *self)\n> > +{\n> > +\tself->queue = gst_atomic_queue_new(4);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraPool *self = GST_LIBCAMERA_POOL(object);\n> > +\tGstBuffer *buf;\n> > +\n> > +\twhile ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))\n> > +\t\tgst_buffer_unref(buf);\n> > +\n> > +\tgst_atomic_queue_unref(self->queue);\n> > +\tg_object_unref(self->allocator);\n> > +\n> > +\tG_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)\n> > +{\n> > +\tauto *object_class = G_OBJECT_CLASS(klass);\n> > +\tauto *pool_class = GST_BUFFER_POOL_CLASS(klass);\n> > +\n> > +\tobject_class->finalize = gst_libcamera_pool_finalize;\n> > +\tpool_class->start = nullptr;\n> > +\tpool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;\n> > +\tpool_class->reset_buffer = gst_libcamera_pool_reset_buffer;\n> > +\tpool_class->release_buffer = gst_libcamera_pool_release_buffer;\n> > +}\n> > +\n> > +GstLibcameraPool *\n> > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)\n> > +{\n> > +\tauto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);\n> > +\n> > +\tpool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);\n> > +\tpool->stream = stream;\n> > +\n> > +\tgsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);\n> > +\tfor (gsize i = 0; i < pool_size; i++) {\n> > +\t\tGstBuffer *buffer = gst_buffer_new();\n> > +\t\tgst_atomic_queue_push(pool->queue, buffer);\n> > +\t}\n> > +\n> > +\treturn pool;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> > new file mode 100644\n> > index 0000000..ca6b299\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerapool.h\n> > @@ -0,0 +1,27 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerapool.h - GStreamer Buffer Pool\n> > + *\n> > + * This is a partial implementation of GstBufferPool intended for internal use\n> > + * only. This pool cannot be configured or activated.\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"gstlibcameraallocator.h\"\n> > +\n> > +#ifndef __GST_LIBCAMERA_POOL_H__\n> > +#define __GST_LIBCAMERA_POOL_H__\n> > +\n> > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,\n> > +\t\t GST_LIBCAMERA, POOL, GstBufferPool)\n> > +\n> > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> > +\t\t\t\t\t libcamera::Stream *stream);\n> > +\n> > +#endif /* __GST_LIBCAMERA_POOL_H__ */\n> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > index e497bf4..346910f 100644\n> > --- a/src/gstreamer/meson.build\n> > +++ b/src/gstreamer/meson.build\n> > @@ -4,6 +4,8 @@ libcamera_gst_sources = [\n> > 'gstlibcamerasrc.cpp',\n> > 'gstlibcameraprovider.cpp',\n> > 'gstlibcamerapad.cpp',\n> > + 'gstlibcameraallocator.cpp',\n> > + 'gstlibcamerapool.cpp'\n> > ]\n> > \n> > libcamera_gst_c_args = [\n> > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [\n> > '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > ]\n> > \n> > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > +gst_req = '>=1.16.1'\n> \n> Maybe gst_dep_version instead of gst_req ?\n> \n> > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,\n> > + required : get_option('gstreamer'))\n> > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,\n> > required : get_option('gstreamer'))\n> > \n> > -if gst_dep.found()\n> > +if gstvideo_dep.found() and gstallocator_dep.found()\n> > libcamera_gst = shared_library('gstlibcamera',\n> > libcamera_gst_sources,\n> > c_args : libcamera_gst_c_args,\n> > include_directories : [],\n> > - dependencies : [libcamera_dep, gst_dep],\n> > + dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],\n> > install: true,\n> > install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > )","headers":{"Return-Path":"","Received":["from mail-qk1-x729.google.com (mail-qk1-x729.google.com\n\t[IPv6:2607:f8b0:4864:20::729])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B79F66042E\n\tfor ;\n\tTue, 25 Feb 2020 21:15:20 +0100 (CET)","by mail-qk1-x729.google.com with SMTP id b5so425432qkh.8\n\tfor ;\n\tTue, 25 Feb 2020 12:15:20 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\td9sm7869513qth.34.2020.02.25.12.15.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 25 Feb 2020 12:15:18 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=otHNeo1n+aDYFZBxgzbdmwE3Ds51EyhXi1lnh56ab8E=;\n\tb=EJvqTzPfa9sOrkGbZvvNu9FNJTis5y0Z1OQ5LefUgQ/OblfUvynyg5B34tMInSgtvq\n\t7A+BKML4rgbLxjFEZ7DAP2jkZVk6+3EhYYHoa53R08nWgU3pYu7gi9jOOCD/HHYQytFO\n\tvdkuUYkjgfy8LB+FdWx3miYMjSd+PA1W48Tcb1iT47q4vdHq0/FNNRFqE0k7JRhkGGQo\n\tVba4p7hOA0a0q20Pzsru8c91qfAB7ggdY37V7PT1prwmZFNvrUr5+nE/9zcwQQ3Jiiyo\n\t4J98f0gmGWqLfe5EFKsaoVQa11CA4Zu4AjdMvIsVAdNpS//R66Ls7U4gHgJXd67ysnrg\n\ttRgw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=otHNeo1n+aDYFZBxgzbdmwE3Ds51EyhXi1lnh56ab8E=;\n\tb=Y4N38L8Ahj2Voqc+6cy3IUVvep873/BOM4gB4yEevwQ2TrE8B2B2Z9Ar1+UfjsGPeJ\n\tPfQeyeN+vVqAiUiy5HDXXdmz/9a3bjkPF8qhs7iUx+KUg8jwE6Tr9bo10etQak9Ujg76\n\tSne8S5qhgseEq2+FBWbPsElvdDbyg/SlTScO4Kx/COP4sIhFdxSEFUqmeO5RSMV5m8ft\n\tMvhxqamYNfNcSGkXEYRML15N45Xr268pZ9pErTgZ1m1s5uYBs4KXRaQTIxV6lsR84rlp\n\tf8Q3IuAJ+A4OnCHZZj7ie3Q/6lvjDT+iYgN9kdmOpq45s68THhCgmS7q8LeqIUiZ5f3B\n\tJ6bQ==","X-Gm-Message-State":"APjAAAVyXrU3LrWHFYmcNQOymF9ITuce2Ng9OFksYIbgI5XAzDiIPiox\n\to3WOLSC61jOv98rgzb03E/KNRQ==","X-Google-Smtp-Source":"APXvYqyzmg0pej6oqbQ1yEDBeYGBGWnpLdfx43PS09V4TmcNzEW7EbnL3XkUk5cVzP4Ia4XS+/VTJA==","X-Received":"by 2002:a37:9d4b:: with SMTP id g72mr705697qke.195.1582661719347;\n\tTue, 25 Feb 2020 12:15:19 -0800 (PST)","Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 25 Feb 2020 15:15:17 -0500","In-Reply-To":"<20200212001625.GQ20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-18-nicolas@ndufresne.ca>\n\t<20200212001625.GQ20823@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 17/23] gst: Add a pool and an\n\tallocator implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 25 Feb 2020 20:15:20 -0000"}}]