[libcamera-devel,v1,17/23] gst: Add a pool and an allocator implementation

2751 diff mbox series

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This is needed to track the livetime of the FrameBufferAllocator in relation to
the GstBuffer/GstMemory objects travelling inside GStreamer.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++
 src/gstreamer/gstlibcameraallocator.h   |  29 +++
 src/gstreamer/gstlibcamerapool.cpp      | 109 +++++++++++
 src/gstreamer/gstlibcamerapool.h        |  27 +++
 src/gstreamer/meson.build               |  11 +-
 5 files changed, 413 insertions(+), 3 deletions(-)
 create mode 100644 src/gstreamer/gstlibcameraallocator.cpp
 create mode 100644 src/gstreamer/gstlibcameraallocator.h
 create mode 100644 src/gstreamer/gstlibcamerapool.cpp
 create mode 100644 src/gstreamer/gstlibcamerapool.h

Hi Nicolas,

Thank you for the patch.

On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This is needed to track the livetime of the FrameBufferAllocator in relation to

s/livetime/lifetime/

> the GstBuffer/GstMemory objects travelling inside GStreamer.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++
>  src/gstreamer/gstlibcameraallocator.h   |  29 +++
>  src/gstreamer/gstlibcamerapool.cpp      | 109 +++++++++++
>  src/gstreamer/gstlibcamerapool.h        |  27 +++
>  src/gstreamer/meson.build               |  11 +-
>  5 files changed, 413 insertions(+), 3 deletions(-)
>  create mode 100644 src/gstreamer/gstlibcameraallocator.cpp
>  create mode 100644 src/gstreamer/gstlibcameraallocator.h
>  create mode 100644 src/gstreamer/gstlibcamerapool.cpp
>  create mode 100644 src/gstreamer/gstlibcamerapool.h
> 
> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> new file mode 100644
> index 0000000..0f248b4
> --- /dev/null
> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> @@ -0,0 +1,240 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Collabora Ltd.
> + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> + *
> + * gstlibcameraallocator.cpp - GStreamer Custom Allocator
> + */
> +
> +#include "gstlibcameraallocator.h"
> +#include "gstlibcamera-utils.h"
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/stream.h>
> +#include <libcamera/framebuffer_allocator.h>

Could you please keep those headers alphabetically sorted ?

> +
> +using namespace libcamera;
> +
> +/***********************************************************************/
> +/* Internal object for tracking memories associated with a FrameBuffer */
> +/***********************************************************************/

Same comment as before regarding this, a doxygen-style comment with a
tiny bit of doc may be useful for inexperienced readers such as your
devoted reviewer :-) You could combine the next comment with it.

> +
> +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);

It would be nice to avoid forward declarations, but it may not be worth
it.

> +
> +/* This internal object is used to track the outstanding GstMemory object that

s/object/objects/

> + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when
> + * al all outstranging GstMemory have returned.

s/al all/all/ ?
s/outstranging/outstanding/ ?

> + */
> +struct FrameWrap {
> +	FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> +		  gpointer stream);
> +	~FrameWrap();
> +
> +	void AcquirePlane() { ++outstanding_planes_; }
> +	bool ReleasePlane() { return --outstanding_planes_ == 0; }

Methods should start with a lowercase letter (camelCase, not CamelCase
as for type names).

> +
> +	gpointer stream_;
> +	FrameBuffer *buffer_;
> +	std::vector<GstMemory *> planes_;
> +	gint outstanding_planes_;

outstandingPlanes_ as it's a C++ class ?

> +};
> +
> +static GQuark
> +gst_libcamera_frame_quark(void)

How about making this a static function of the FrameWrap class ?

> +{
> +	static gsize frame_quark = 0;
> +
> +	if (g_once_init_enter(&frame_quark)) {
> +		GQuark quark = g_quark_from_string("GstLibCameraFrameWrap");
> +		g_once_init_leave(&frame_quark, quark);
> +	}
> +
> +	return frame_quark;
> +}
> +
> +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> +		     gpointer stream)
> +
> +	: stream_(stream),
> +	  buffer_(buffer),
> +	  outstanding_planes_(0)
> +{
> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +		GstMemory *mem = gst_fd_allocator_alloc(allocator,
> +							plane.fd.fd(),
> +							plane.length,
> +							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> +		gst_mini_object_set_qdata(GST_MINI_OBJECT(mem),
> +					  gst_libcamera_frame_quark(),
> +					  this, NULL);
> +		GST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;
> +		g_object_unref(mem->allocator);
> +		planes_.push_back(mem);
> +	}
> +}
> +
> +FrameWrap::~FrameWrap()
> +{
> +	for (GstMemory *mem : planes_) {
> +		GST_MINI_OBJECT(mem)->dispose = nullptr;
> +		g_object_ref(mem->allocator);
> +		gst_memory_unref(mem);
> +	}
> +}
> +
> +/***********************************/
> +/* The GstAllocator implementation */
> +/***********************************/
> +struct _GstLibcameraAllocator {
> +	GstDmaBufAllocator parent;
> +	FrameBufferAllocator *fb_allocator;
> +	/* A hash table using Stream pointer as key and returning a GQueue of
> +	 * FrameWrap. */
> +	GHashTable *pools;
> +};
> +
> +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> +	      GST_TYPE_DMABUF_ALLOCATOR);
> +
> +static gboolean
> +gst_libcamera_allocator_release(GstMiniObject *mini_object)
> +{
> +	GstMemory *mem = GST_MEMORY_CAST(mini_object);
> +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);
> +	GST_OBJECT_LOCKER(self);
> +	gpointer data = gst_mini_object_get_qdata(mini_object,
> +						  gst_libcamera_frame_quark());
> +	auto *frame = (FrameWrap *)data;

Could you use reinterpret_cast<FrameWrap *>(data) here ? And maybe
replace auto with FrameWrap as it's short.

In general, please use static_cast or reinterpret_cast as appropriate,
as they offer more compile-time protection that C-style casts. I won't
comment on other occurrences.

> +
> +	gst_memory_ref(mem);
> +
> +	if (frame->ReleasePlane()) {
> +		auto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);
> +		g_return_val_if_fail(pool, TRUE);
> +		g_queue_push_tail(pool, frame);
> +	}
> +
> +	/* Keep last in case we are holding on the last allocator ref */
> +	g_object_unref(mem->allocator);
> +
> +	/* Rreturns FALSE so that our mini object isn't freed */

s/Rreturns/Return/

> +	return FALSE;
> +}
> +
> +static void
> +gst_libcamera_allocator_free_pool(gpointer data)
> +{
> +	GQueue *queue = (GQueue *)data;
> +	FrameWrap *frame;
> +
> +	while ((frame = (FrameWrap *)g_queue_pop_head(queue))) {
> +		g_warn_if_fail(frame->outstanding_planes_ == 0);
> +		delete frame;
> +	}
> +
> +	g_queue_free(queue);
> +}
> +
> +static void
> +gst_libcamera_allocator_init(GstLibcameraAllocator *self)
> +{
> +	self->pools = g_hash_table_new_full(NULL, NULL, NULL,
> +					    gst_libcamera_allocator_free_pool);
> +	GST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);
> +}
> +
> +static void
> +gst_libcamera_allocator_dispose(GObject *object)
> +{
> +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> +
> +	if (self->pools) {
> +		g_hash_table_unref(self->pools);
> +		self->pools = NULL;
> +	}
> +
> +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);
> +}
> +
> +static void
> +gst_libcamera_allocator_finalize(GObject *object)
> +{
> +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> +
> +	delete self->fb_allocator;
> +
> +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);
> +}
> +
> +static void
> +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)
> +{
> +	auto *allocator_class = GST_ALLOCATOR_CLASS(klass);
> +	auto *object_class = G_OBJECT_CLASS(klass);
> +
> +	object_class->dispose = gst_libcamera_allocator_dispose;
> +	object_class->finalize = gst_libcamera_allocator_finalize;
> +	allocator_class->alloc = NULL;
> +}
> +
> +GstLibcameraAllocator *
> +gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
> +{
> +	auto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> +							   nullptr);
> +
> +	self->fb_allocator = FrameBufferAllocator::create(camera);
> +	for (Stream *stream : camera->streams()) {
> +		gint ret;
> +
> +		ret = self->fb_allocator->allocate(stream);
> +		if (ret == 0)
> +			return nullptr;
> +
> +		GQueue *pool = g_queue_new();
> +		for (const std::unique_ptr<FrameBuffer> &buffer :
> +		     self->fb_allocator->buffers(stream)) {
> +			auto *fb = new FrameWrap(GST_ALLOCATOR(self),
> +						 buffer.get(), stream);
> +			g_queue_push_tail(pool, fb);
> +		}
> +
> +		g_hash_table_insert(self->pools, stream, pool);
> +	}
> +
> +	return self;
> +}
> +
> +bool
> +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> +				       Stream *stream, GstBuffer *buffer)
> +{
> +	GST_OBJECT_LOCKER(self);
> +
> +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> +	g_return_val_if_fail(pool, false);
> +
> +	auto *frame = (FrameWrap *)g_queue_pop_head(pool);
> +	if (!frame)
> +		return false;
> +
> +	for (GstMemory *mem : frame->planes_) {
> +		frame->AcquirePlane();
> +		gst_buffer_append_memory(buffer, mem);
> +		g_object_ref(mem->allocator);
> +	}
> +
> +	return true;
> +}
> +
> +gsize
> +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,
> +				      Stream *stream)
> +{
> +	GST_OBJECT_LOCKER(self);
> +
> +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> +	g_return_val_if_fail(pool, false);
> +
> +	return pool->length;
> +}
> diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h
> new file mode 100644
> index 0000000..f2a0f58
> --- /dev/null
> +++ b/src/gstreamer/gstlibcameraallocator.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Collabora Ltd.
> + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> + *
> + * gstlibcameraallocator.h - GStreamer Custom Allocator
> + */
> +
> +#include <gst/gst.h>
> +#include <gst/allocators/allocators.h>
> +#include <libcamera/stream.h>
> +
> +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__
> +#define __GST_LIBCAMERA_ALLOCATOR_H__
> +
> +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()
> +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> +		     GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)
> +
> +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera);
> +
> +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> +					    libcamera::Stream *stream,
> +					    GstBuffer *buffer);
> +
> +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,
> +					    libcamera::Stream *stream);
> +
> +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */
> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> new file mode 100644
> index 0000000..f84d1d6
> --- /dev/null
> +++ b/src/gstreamer/gstlibcamerapool.cpp
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Collabora Ltd.
> + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> + *
> + * gstlibcamerapool.cpp - GStreamer Buffer Pool
> + */
> +
> +#include "gstlibcamerapool.h"
> +#include "gstlibcamera-utils.h"
> +
> +#include <libcamera/stream.h>
> +
> +using namespace libcamera;
> +
> +struct _GstLibcameraPool {
> +	GstBufferPool parent;
> +
> +	GstAtomicQueue *queue;
> +	GstLibcameraAllocator *allocator;
> +	Stream *stream;
> +};
> +
> +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);
> +
> +static GstFlowReturn
> +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,
> +				  GstBufferPoolAcquireParams *params)
> +{
> +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> +	GstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);
> +	if (!buf)
> +		return GST_FLOW_ERROR;
> +
> +	if (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))
> +		return GST_FLOW_ERROR;
> +
> +	*buffer = buf;
> +	return GST_FLOW_OK;
> +}
> +
> +static void
> +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)
> +{
> +	GstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);
> +
> +	/* Clears all the memories and only pool the GstBuffer objects */
> +	gst_buffer_remove_all_memory(buffer);
> +	klass->reset_buffer(pool, buffer);
> +	GST_BUFFER_FLAGS(buffer) = 0;
> +}
> +
> +static void
> +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)
> +{
> +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> +	gst_atomic_queue_push(self->queue, (gpointer)buffer);
> +}
> +
> +static void
> +gst_libcamera_pool_init(GstLibcameraPool *self)
> +{
> +	self->queue = gst_atomic_queue_new(4);
> +}
> +
> +static void
> +gst_libcamera_pool_finalize(GObject *object)
> +{
> +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(object);
> +	GstBuffer *buf;
> +
> +	while ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))
> +		gst_buffer_unref(buf);
> +
> +	gst_atomic_queue_unref(self->queue);
> +	g_object_unref(self->allocator);
> +
> +	G_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);
> +}
> +
> +static void
> +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> +{
> +	auto *object_class = G_OBJECT_CLASS(klass);
> +	auto *pool_class = GST_BUFFER_POOL_CLASS(klass);
> +
> +	object_class->finalize = gst_libcamera_pool_finalize;
> +	pool_class->start = nullptr;
> +	pool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;
> +	pool_class->reset_buffer = gst_libcamera_pool_reset_buffer;
> +	pool_class->release_buffer = gst_libcamera_pool_release_buffer;
> +}
> +
> +GstLibcameraPool *
> +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> +{
> +	auto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);
> +
> +	pool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);
> +	pool->stream = stream;
> +
> +	gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> +	for (gsize i = 0; i < pool_size; i++) {
> +		GstBuffer *buffer = gst_buffer_new();
> +		gst_atomic_queue_push(pool->queue, buffer);
> +	}
> +
> +	return pool;
> +}
> diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> new file mode 100644
> index 0000000..ca6b299
> --- /dev/null
> +++ b/src/gstreamer/gstlibcamerapool.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Collabora Ltd.
> + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> + *
> + * gstlibcamerapool.h - GStreamer Buffer Pool
> + *
> + * This is a partial implementation of GstBufferPool intended for internal use
> + * only. This pool cannot be configured or activated.
> + */
> +
> +#include <gst/gst.h>
> +#include <libcamera/stream.h>
> +
> +#include "gstlibcameraallocator.h"
> +
> +#ifndef __GST_LIBCAMERA_POOL_H__
> +#define __GST_LIBCAMERA_POOL_H__
> +
> +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()
> +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,
> +		     GST_LIBCAMERA, POOL, GstBufferPool)
> +
> +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> +					 libcamera::Stream *stream);
> +
> +#endif /* __GST_LIBCAMERA_POOL_H__ */
> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> index e497bf4..346910f 100644
> --- a/src/gstreamer/meson.build
> +++ b/src/gstreamer/meson.build
> @@ -4,6 +4,8 @@ libcamera_gst_sources = [
>      'gstlibcamerasrc.cpp',
>      'gstlibcameraprovider.cpp',
>      'gstlibcamerapad.cpp',
> +    'gstlibcameraallocator.cpp',
> +    'gstlibcamerapool.cpp'
>  ]
>  
>  libcamera_gst_c_args = [
> @@ -11,15 +13,18 @@ libcamera_gst_c_args = [
>      '-DPACKAGE="@0@"'.format(meson.project_name()),
>  ]
>  
> -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',
> +gst_req = '>=1.16.1'

Maybe gst_dep_version instead of gst_req ?

> +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,
> +    required : get_option('gstreamer'))
> +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,
>      required : get_option('gstreamer'))
>  
> -if gst_dep.found()
> +if gstvideo_dep.found() and gstallocator_dep.found()
>    libcamera_gst = shared_library('gstlibcamera',
>        libcamera_gst_sources,
>        c_args : libcamera_gst_c_args,
>        include_directories : [],
> -      dependencies : [libcamera_dep, gst_dep],
> +      dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],
>        install: true,
>        install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
>    )

Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > This is needed to track the livetime of the FrameBufferAllocator in relation to
> 
> s/livetime/lifetime/
> 
> > the GstBuffer/GstMemory objects travelling inside GStreamer.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++
> >  src/gstreamer/gstlibcameraallocator.h   |  29 +++
> >  src/gstreamer/gstlibcamerapool.cpp      | 109 +++++++++++
> >  src/gstreamer/gstlibcamerapool.h        |  27 +++
> >  src/gstreamer/meson.build               |  11 +-
> >  5 files changed, 413 insertions(+), 3 deletions(-)
> >  create mode 100644 src/gstreamer/gstlibcameraallocator.cpp
> >  create mode 100644 src/gstreamer/gstlibcameraallocator.h
> >  create mode 100644 src/gstreamer/gstlibcamerapool.cpp
> >  create mode 100644 src/gstreamer/gstlibcamerapool.h
> > 
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > new file mode 100644
> > index 0000000..0f248b4
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -0,0 +1,240 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator
> > + */
> > +
> > +#include "gstlibcameraallocator.h"
> > +#include "gstlibcamera-utils.h"
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/stream.h>
> > +#include <libcamera/framebuffer_allocator.h>
> 
> Could you please keep those headers alphabetically sorted ?
> 
> > +
> > +using namespace libcamera;
> > +
> > +/***********************************************************************/
> > +/* Internal object for tracking memories associated with a FrameBuffer */
> > +/***********************************************************************/
> 
> Same comment as before regarding this, a doxygen-style comment with a
> tiny bit of doc may be useful for inexperienced readers such as your
> devoted reviewer :-) You could combine the next comment with it.

Ok, this was just a banner to help me visually locate the two objects
in this .cpp file. I didn't know it was not allowed in this project. I
can add more information of course, I'm not familiar with doxygen doc,
I would not make that part of the generated doc though, it's internal.

> 
> > +
> > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);
> 
> It would be nice to avoid forward declarations, but it may not be worth
> it.
> 
> > +
> > +/* This internal object is used to track the outstanding GstMemory object that
> 
> s/object/objects/
> 
> > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when
> > + * al all outstranging GstMemory have returned.
> 
> s/al all/all/ ?
> s/outstranging/outstanding/ ?
> 
> > + */
> > +struct FrameWrap {
> > +	FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > +		  gpointer stream);
> > +	~FrameWrap();
> > +
> > +	void AcquirePlane() { ++outstanding_planes_; }
> > +	bool ReleasePlane() { return --outstanding_planes_ == 0; }
> 
> Methods should start with a lowercase letter (camelCase, not CamelCase
> as for type names).
> 
> > +
> > +	gpointer stream_;
> > +	FrameBuffer *buffer_;
> > +	std::vector<GstMemory *> planes_;
> > +	gint outstanding_planes_;
> 
> outstandingPlanes_ as it's a C++ class ?
> 
> > +};
> > +
> > +static GQuark
> > +gst_libcamera_frame_quark(void)
> 
> How about making this a static function of the FrameWrap class ?
> 
> > +{
> > +	static gsize frame_quark = 0;
> > +
> > +	if (g_once_init_enter(&frame_quark)) {
> > +		GQuark quark = g_quark_from_string("GstLibCameraFrameWrap");
> > +		g_once_init_leave(&frame_quark, quark);
> > +	}
> > +
> > +	return frame_quark;
> > +}
> > +
> > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > +		     gpointer stream)
> > +
> > +	: stream_(stream),
> > +	  buffer_(buffer),
> > +	  outstanding_planes_(0)
> > +{
> > +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > +		GstMemory *mem = gst_fd_allocator_alloc(allocator,
> > +							plane.fd.fd(),
> > +							plane.length,
> > +							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> > +		gst_mini_object_set_qdata(GST_MINI_OBJECT(mem),
> > +					  gst_libcamera_frame_quark(),
> > +					  this, NULL);
> > +		GST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;
> > +		g_object_unref(mem->allocator);
> > +		planes_.push_back(mem);
> > +	}
> > +}
> > +
> > +FrameWrap::~FrameWrap()
> > +{
> > +	for (GstMemory *mem : planes_) {
> > +		GST_MINI_OBJECT(mem)->dispose = nullptr;
> > +		g_object_ref(mem->allocator);
> > +		gst_memory_unref(mem);
> > +	}
> > +}
> > +
> > +/***********************************/
> > +/* The GstAllocator implementation */
> > +/***********************************/

Needed here to.

> > +struct _GstLibcameraAllocator {
> > +	GstDmaBufAllocator parent;
> > +	FrameBufferAllocator *fb_allocator;
> > +	/* A hash table using Stream pointer as key and returning a GQueue of
> > +	 * FrameWrap. */
> > +	GHashTable *pools;
> > +};
> > +
> > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > +	      GST_TYPE_DMABUF_ALLOCATOR);
> > +
> > +static gboolean
> > +gst_libcamera_allocator_release(GstMiniObject *mini_object)
> > +{
> > +	GstMemory *mem = GST_MEMORY_CAST(mini_object);
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);
> > +	GST_OBJECT_LOCKER(self);
> > +	gpointer data = gst_mini_object_get_qdata(mini_object,
> > +						  gst_libcamera_frame_quark());
> > +	auto *frame = (FrameWrap *)data;
> 
> Could you use reinterpret_cast<FrameWrap *>(data) here ? And maybe
> replace auto with FrameWrap as it's short.
> 
> In general, please use static_cast or reinterpret_cast as appropriate,
> as they offer more compile-time protection that C-style casts. I won't
> comment on other occurrences.
> 
> > +
> > +	gst_memory_ref(mem);
> > +
> > +	if (frame->ReleasePlane()) {
> > +		auto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);
> > +		g_return_val_if_fail(pool, TRUE);
> > +		g_queue_push_tail(pool, frame);
> > +	}
> > +
> > +	/* Keep last in case we are holding on the last allocator ref */
> > +	g_object_unref(mem->allocator);
> > +
> > +	/* Rreturns FALSE so that our mini object isn't freed */
> 
> s/Rreturns/Return/
> 
> > +	return FALSE;
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_free_pool(gpointer data)
> > +{
> > +	GQueue *queue = (GQueue *)data;
> > +	FrameWrap *frame;
> > +
> > +	while ((frame = (FrameWrap *)g_queue_pop_head(queue))) {
> > +		g_warn_if_fail(frame->outstanding_planes_ == 0);
> > +		delete frame;
> > +	}
> > +
> > +	g_queue_free(queue);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)
> > +{
> > +	self->pools = g_hash_table_new_full(NULL, NULL, NULL,
> > +					    gst_libcamera_allocator_free_pool);
> > +	GST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_dispose(GObject *object)
> > +{
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > +
> > +	if (self->pools) {
> > +		g_hash_table_unref(self->pools);
> > +		self->pools = NULL;
> > +	}
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_finalize(GObject *object)
> > +{
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > +
> > +	delete self->fb_allocator;
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)
> > +{
> > +	auto *allocator_class = GST_ALLOCATOR_CLASS(klass);
> > +	auto *object_class = G_OBJECT_CLASS(klass);
> > +
> > +	object_class->dispose = gst_libcamera_allocator_dispose;
> > +	object_class->finalize = gst_libcamera_allocator_finalize;
> > +	allocator_class->alloc = NULL;
> > +}
> > +
> > +GstLibcameraAllocator *
> > +gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
> > +{
> > +	auto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > +							   nullptr);
> > +
> > +	self->fb_allocator = FrameBufferAllocator::create(camera);
> > +	for (Stream *stream : camera->streams()) {
> > +		gint ret;
> > +
> > +		ret = self->fb_allocator->allocate(stream);
> > +		if (ret == 0)
> > +			return nullptr;
> > +
> > +		GQueue *pool = g_queue_new();
> > +		for (const std::unique_ptr<FrameBuffer> &buffer :
> > +		     self->fb_allocator->buffers(stream)) {
> > +			auto *fb = new FrameWrap(GST_ALLOCATOR(self),
> > +						 buffer.get(), stream);
> > +			g_queue_push_tail(pool, fb);
> > +		}
> > +
> > +		g_hash_table_insert(self->pools, stream, pool);
> > +	}
> > +
> > +	return self;
> > +}
> > +
> > +bool
> > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > +				       Stream *stream, GstBuffer *buffer)
> > +{
> > +	GST_OBJECT_LOCKER(self);
> > +
> > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > +	g_return_val_if_fail(pool, false);
> > +
> > +	auto *frame = (FrameWrap *)g_queue_pop_head(pool);
> > +	if (!frame)
> > +		return false;
> > +
> > +	for (GstMemory *mem : frame->planes_) {
> > +		frame->AcquirePlane();
> > +		gst_buffer_append_memory(buffer, mem);
> > +		g_object_ref(mem->allocator);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +gsize
> > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,
> > +				      Stream *stream)
> > +{
> > +	GST_OBJECT_LOCKER(self);
> > +
> > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > +	g_return_val_if_fail(pool, false);
> > +
> > +	return pool->length;
> > +}
> > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h
> > new file mode 100644
> > index 0000000..f2a0f58
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraallocator.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcameraallocator.h - GStreamer Custom Allocator
> > + */
> > +
> > +#include <gst/gst.h>
> > +#include <gst/allocators/allocators.h>
> > +#include <libcamera/stream.h>
> > +
> > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__
> > +#define __GST_LIBCAMERA_ALLOCATOR_H__
> > +
> > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > +		     GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)
> > +
> > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera);
> > +
> > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > +					    libcamera::Stream *stream,
> > +					    GstBuffer *buffer);
> > +
> > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,
> > +					    libcamera::Stream *stream);
> > +
> > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */
> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> > new file mode 100644
> > index 0000000..f84d1d6
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > @@ -0,0 +1,109 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcamerapool.cpp - GStreamer Buffer Pool
> > + */
> > +
> > +#include "gstlibcamerapool.h"
> > +#include "gstlibcamera-utils.h"
> > +
> > +#include <libcamera/stream.h>
> > +
> > +using namespace libcamera;
> > +
> > +struct _GstLibcameraPool {
> > +	GstBufferPool parent;
> > +
> > +	GstAtomicQueue *queue;
> > +	GstLibcameraAllocator *allocator;
> > +	Stream *stream;
> > +};
> > +
> > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);
> > +
> > +static GstFlowReturn
> > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,
> > +				  GstBufferPoolAcquireParams *params)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > +	GstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);
> > +	if (!buf)
> > +		return GST_FLOW_ERROR;
> > +
> > +	if (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))
> > +		return GST_FLOW_ERROR;
> > +
> > +	*buffer = buf;
> > +	return GST_FLOW_OK;
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > +{
> > +	GstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);
> > +
> > +	/* Clears all the memories and only pool the GstBuffer objects */
> > +	gst_buffer_remove_all_memory(buffer);
> > +	klass->reset_buffer(pool, buffer);
> > +	GST_BUFFER_FLAGS(buffer) = 0;
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > +	gst_atomic_queue_push(self->queue, (gpointer)buffer);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_init(GstLibcameraPool *self)
> > +{
> > +	self->queue = gst_atomic_queue_new(4);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_finalize(GObject *object)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(object);
> > +	GstBuffer *buf;
> > +
> > +	while ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))
> > +		gst_buffer_unref(buf);
> > +
> > +	gst_atomic_queue_unref(self->queue);
> > +	g_object_unref(self->allocator);
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> > +{
> > +	auto *object_class = G_OBJECT_CLASS(klass);
> > +	auto *pool_class = GST_BUFFER_POOL_CLASS(klass);
> > +
> > +	object_class->finalize = gst_libcamera_pool_finalize;
> > +	pool_class->start = nullptr;
> > +	pool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;
> > +	pool_class->reset_buffer = gst_libcamera_pool_reset_buffer;
> > +	pool_class->release_buffer = gst_libcamera_pool_release_buffer;
> > +}
> > +
> > +GstLibcameraPool *
> > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> > +{
> > +	auto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);
> > +
> > +	pool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);
> > +	pool->stream = stream;
> > +
> > +	gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> > +	for (gsize i = 0; i < pool_size; i++) {
> > +		GstBuffer *buffer = gst_buffer_new();
> > +		gst_atomic_queue_push(pool->queue, buffer);
> > +	}
> > +
> > +	return pool;
> > +}
> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> > new file mode 100644
> > index 0000000..ca6b299
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcamerapool.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcamerapool.h - GStreamer Buffer Pool
> > + *
> > + * This is a partial implementation of GstBufferPool intended for internal use
> > + * only. This pool cannot be configured or activated.
> > + */
> > +
> > +#include <gst/gst.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "gstlibcameraallocator.h"
> > +
> > +#ifndef __GST_LIBCAMERA_POOL_H__
> > +#define __GST_LIBCAMERA_POOL_H__
> > +
> > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,
> > +		     GST_LIBCAMERA, POOL, GstBufferPool)
> > +
> > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> > +					 libcamera::Stream *stream);
> > +
> > +#endif /* __GST_LIBCAMERA_POOL_H__ */
> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > index e497bf4..346910f 100644
> > --- a/src/gstreamer/meson.build
> > +++ b/src/gstreamer/meson.build
> > @@ -4,6 +4,8 @@ libcamera_gst_sources = [
> >      'gstlibcamerasrc.cpp',
> >      'gstlibcameraprovider.cpp',
> >      'gstlibcamerapad.cpp',
> > +    'gstlibcameraallocator.cpp',
> > +    'gstlibcamerapool.cpp'
> >  ]
> >  
> >  libcamera_gst_c_args = [
> > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [
> >      '-DPACKAGE="@0@"'.format(meson.project_name()),
> >  ]
> >  
> > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',
> > +gst_req = '>=1.16.1'
> 
> Maybe gst_dep_version instead of gst_req ?
> 
> > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,
> > +    required : get_option('gstreamer'))
> > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,
> >      required : get_option('gstreamer'))
> >  
> > -if gst_dep.found()
> > +if gstvideo_dep.found() and gstallocator_dep.found()
> >    libcamera_gst = shared_library('gstlibcamera',
> >        libcamera_gst_sources,
> >        c_args : libcamera_gst_c_args,
> >        include_directories : [],
> > -      dependencies : [libcamera_dep, gst_dep],
> > +      dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],
> >        install: true,
> >        install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
> >    )

Hi Nicolas,

On Wed, Feb 12, 2020 at 12:37:50PM -0500, Nicolas Dufresne wrote:
> Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :
> > On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > 
> > > This is needed to track the livetime of the FrameBufferAllocator in relation to
> > 
> > s/livetime/lifetime/
> > 
> > > the GstBuffer/GstMemory objects travelling inside GStreamer.
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > >  src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++
> > >  src/gstreamer/gstlibcameraallocator.h   |  29 +++
> > >  src/gstreamer/gstlibcamerapool.cpp      | 109 +++++++++++
> > >  src/gstreamer/gstlibcamerapool.h        |  27 +++
> > >  src/gstreamer/meson.build               |  11 +-
> > >  5 files changed, 413 insertions(+), 3 deletions(-)
> > >  create mode 100644 src/gstreamer/gstlibcameraallocator.cpp
> > >  create mode 100644 src/gstreamer/gstlibcameraallocator.h
> > >  create mode 100644 src/gstreamer/gstlibcamerapool.cpp
> > >  create mode 100644 src/gstreamer/gstlibcamerapool.h
> > > 
> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > > new file mode 100644
> > > index 0000000..0f248b4
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > > @@ -0,0 +1,240 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > + *
> > > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator
> > > + */
> > > +
> > > +#include "gstlibcameraallocator.h"
> > > +#include "gstlibcamera-utils.h"
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/stream.h>
> > > +#include <libcamera/framebuffer_allocator.h>
> > 
> > Could you please keep those headers alphabetically sorted ?
> > 
> > > +
> > > +using namespace libcamera;
> > > +
> > > +/***********************************************************************/
> > > +/* Internal object for tracking memories associated with a FrameBuffer */
> > > +/***********************************************************************/
> > 
> > Same comment as before regarding this, a doxygen-style comment with a
> > tiny bit of doc may be useful for inexperienced readers such as your
> > devoted reviewer :-) You could combine the next comment with it.
> 
> Ok, this was just a banner to help me visually locate the two objects
> in this .cpp file. I didn't know it was not allowed in this project. I
> can add more information of course, I'm not familiar with doxygen doc,
> I would not make that part of the generated doc though, it's internal.

I wouldn't go as far as saying it's not allowed :-) I only wanted to
point out that a comment along the lines of

/**
 * \struct FrameWrap
 * \brief Track memories associated with a FrameBuffer
 *
 * This structure .....
 * .....
 * .....
 */

could provide a bit of useful information to the reader and serve as a
visual separator. Please pick the style you like the most.

> > > +
> > > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);
> > 
> > It would be nice to avoid forward declarations, but it may not be worth
> > it.
> > 
> > > +
> > > +/* This internal object is used to track the outstanding GstMemory object that
> > 
> > s/object/objects/
> > 
> > > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when
> > > + * al all outstranging GstMemory have returned.
> > 
> > s/al all/all/ ?
> > s/outstranging/outstanding/ ?
> > 
> > > + */
> > > +struct FrameWrap {
> > > +	FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > > +		  gpointer stream);
> > > +	~FrameWrap();
> > > +
> > > +	void AcquirePlane() { ++outstanding_planes_; }
> > > +	bool ReleasePlane() { return --outstanding_planes_ == 0; }
> > 
> > Methods should start with a lowercase letter (camelCase, not CamelCase
> > as for type names).
> > 
> > > +
> > > +	gpointer stream_;
> > > +	FrameBuffer *buffer_;
> > > +	std::vector<GstMemory *> planes_;
> > > +	gint outstanding_planes_;
> > 
> > outstandingPlanes_ as it's a C++ class ?
> > 
> > > +};
> > > +
> > > +static GQuark
> > > +gst_libcamera_frame_quark(void)
> > 
> > How about making this a static function of the FrameWrap class ?
> > 
> > > +{
> > > +	static gsize frame_quark = 0;
> > > +
> > > +	if (g_once_init_enter(&frame_quark)) {
> > > +		GQuark quark = g_quark_from_string("GstLibCameraFrameWrap");
> > > +		g_once_init_leave(&frame_quark, quark);
> > > +	}
> > > +
> > > +	return frame_quark;
> > > +}
> > > +
> > > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > > +		     gpointer stream)
> > > +
> > > +	: stream_(stream),
> > > +	  buffer_(buffer),
> > > +	  outstanding_planes_(0)
> > > +{
> > > +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > > +		GstMemory *mem = gst_fd_allocator_alloc(allocator,
> > > +							plane.fd.fd(),
> > > +							plane.length,
> > > +							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> > > +		gst_mini_object_set_qdata(GST_MINI_OBJECT(mem),
> > > +					  gst_libcamera_frame_quark(),
> > > +					  this, NULL);
> > > +		GST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;
> > > +		g_object_unref(mem->allocator);
> > > +		planes_.push_back(mem);
> > > +	}
> > > +}
> > > +
> > > +FrameWrap::~FrameWrap()
> > > +{
> > > +	for (GstMemory *mem : planes_) {
> > > +		GST_MINI_OBJECT(mem)->dispose = nullptr;
> > > +		g_object_ref(mem->allocator);
> > > +		gst_memory_unref(mem);
> > > +	}
> > > +}
> > > +
> > > +/***********************************/
> > > +/* The GstAllocator implementation */
> > > +/***********************************/
> 
> Needed here to.
> 
> > > +struct _GstLibcameraAllocator {
> > > +	GstDmaBufAllocator parent;
> > > +	FrameBufferAllocator *fb_allocator;
> > > +	/* A hash table using Stream pointer as key and returning a GQueue of
> > > +	 * FrameWrap. */
> > > +	GHashTable *pools;
> > > +};
> > > +
> > > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > > +	      GST_TYPE_DMABUF_ALLOCATOR);
> > > +
> > > +static gboolean
> > > +gst_libcamera_allocator_release(GstMiniObject *mini_object)
> > > +{
> > > +	GstMemory *mem = GST_MEMORY_CAST(mini_object);
> > > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);
> > > +	GST_OBJECT_LOCKER(self);
> > > +	gpointer data = gst_mini_object_get_qdata(mini_object,
> > > +						  gst_libcamera_frame_quark());
> > > +	auto *frame = (FrameWrap *)data;
> > 
> > Could you use reinterpret_cast<FrameWrap *>(data) here ? And maybe
> > replace auto with FrameWrap as it's short.
> > 
> > In general, please use static_cast or reinterpret_cast as appropriate,
> > as they offer more compile-time protection that C-style casts. I won't
> > comment on other occurrences.
> > 
> > > +
> > > +	gst_memory_ref(mem);
> > > +
> > > +	if (frame->ReleasePlane()) {
> > > +		auto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);
> > > +		g_return_val_if_fail(pool, TRUE);
> > > +		g_queue_push_tail(pool, frame);
> > > +	}
> > > +
> > > +	/* Keep last in case we are holding on the last allocator ref */
> > > +	g_object_unref(mem->allocator);
> > > +
> > > +	/* Rreturns FALSE so that our mini object isn't freed */
> > 
> > s/Rreturns/Return/
> > 
> > > +	return FALSE;
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_allocator_free_pool(gpointer data)
> > > +{
> > > +	GQueue *queue = (GQueue *)data;
> > > +	FrameWrap *frame;
> > > +
> > > +	while ((frame = (FrameWrap *)g_queue_pop_head(queue))) {
> > > +		g_warn_if_fail(frame->outstanding_planes_ == 0);
> > > +		delete frame;
> > > +	}
> > > +
> > > +	g_queue_free(queue);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)
> > > +{
> > > +	self->pools = g_hash_table_new_full(NULL, NULL, NULL,
> > > +					    gst_libcamera_allocator_free_pool);
> > > +	GST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_allocator_dispose(GObject *object)
> > > +{
> > > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > > +
> > > +	if (self->pools) {
> > > +		g_hash_table_unref(self->pools);
> > > +		self->pools = NULL;
> > > +	}
> > > +
> > > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_allocator_finalize(GObject *object)
> > > +{
> > > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > > +
> > > +	delete self->fb_allocator;
> > > +
> > > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)
> > > +{
> > > +	auto *allocator_class = GST_ALLOCATOR_CLASS(klass);
> > > +	auto *object_class = G_OBJECT_CLASS(klass);
> > > +
> > > +	object_class->dispose = gst_libcamera_allocator_dispose;
> > > +	object_class->finalize = gst_libcamera_allocator_finalize;
> > > +	allocator_class->alloc = NULL;
> > > +}
> > > +
> > > +GstLibcameraAllocator *
> > > +gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
> > > +{
> > > +	auto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > > +							   nullptr);
> > > +
> > > +	self->fb_allocator = FrameBufferAllocator::create(camera);
> > > +	for (Stream *stream : camera->streams()) {
> > > +		gint ret;
> > > +
> > > +		ret = self->fb_allocator->allocate(stream);
> > > +		if (ret == 0)
> > > +			return nullptr;
> > > +
> > > +		GQueue *pool = g_queue_new();
> > > +		for (const std::unique_ptr<FrameBuffer> &buffer :
> > > +		     self->fb_allocator->buffers(stream)) {
> > > +			auto *fb = new FrameWrap(GST_ALLOCATOR(self),
> > > +						 buffer.get(), stream);
> > > +			g_queue_push_tail(pool, fb);
> > > +		}
> > > +
> > > +		g_hash_table_insert(self->pools, stream, pool);
> > > +	}
> > > +
> > > +	return self;
> > > +}
> > > +
> > > +bool
> > > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > > +				       Stream *stream, GstBuffer *buffer)
> > > +{
> > > +	GST_OBJECT_LOCKER(self);
> > > +
> > > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > > +	g_return_val_if_fail(pool, false);
> > > +
> > > +	auto *frame = (FrameWrap *)g_queue_pop_head(pool);
> > > +	if (!frame)
> > > +		return false;
> > > +
> > > +	for (GstMemory *mem : frame->planes_) {
> > > +		frame->AcquirePlane();
> > > +		gst_buffer_append_memory(buffer, mem);
> > > +		g_object_ref(mem->allocator);
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +gsize
> > > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,
> > > +				      Stream *stream)
> > > +{
> > > +	GST_OBJECT_LOCKER(self);
> > > +
> > > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > > +	g_return_val_if_fail(pool, false);
> > > +
> > > +	return pool->length;
> > > +}
> > > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h
> > > new file mode 100644
> > > index 0000000..f2a0f58
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcameraallocator.h
> > > @@ -0,0 +1,29 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > + *
> > > + * gstlibcameraallocator.h - GStreamer Custom Allocator
> > > + */
> > > +
> > > +#include <gst/gst.h>
> > > +#include <gst/allocators/allocators.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__
> > > +#define __GST_LIBCAMERA_ALLOCATOR_H__
> > > +
> > > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()
> > > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > > +		     GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)
> > > +
> > > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera);
> > > +
> > > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > > +					    libcamera::Stream *stream,
> > > +					    GstBuffer *buffer);
> > > +
> > > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,
> > > +					    libcamera::Stream *stream);
> > > +
> > > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */
> > > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> > > new file mode 100644
> > > index 0000000..f84d1d6
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > > @@ -0,0 +1,109 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > + *
> > > + * gstlibcamerapool.cpp - GStreamer Buffer Pool
> > > + */
> > > +
> > > +#include "gstlibcamerapool.h"
> > > +#include "gstlibcamera-utils.h"
> > > +
> > > +#include <libcamera/stream.h>
> > > +
> > > +using namespace libcamera;
> > > +
> > > +struct _GstLibcameraPool {
> > > +	GstBufferPool parent;
> > > +
> > > +	GstAtomicQueue *queue;
> > > +	GstLibcameraAllocator *allocator;
> > > +	Stream *stream;
> > > +};
> > > +
> > > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);
> > > +
> > > +static GstFlowReturn
> > > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,
> > > +				  GstBufferPoolAcquireParams *params)
> > > +{
> > > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > > +	GstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);
> > > +	if (!buf)
> > > +		return GST_FLOW_ERROR;
> > > +
> > > +	if (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))
> > > +		return GST_FLOW_ERROR;
> > > +
> > > +	*buffer = buf;
> > > +	return GST_FLOW_OK;
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > > +{
> > > +	GstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);
> > > +
> > > +	/* Clears all the memories and only pool the GstBuffer objects */
> > > +	gst_buffer_remove_all_memory(buffer);
> > > +	klass->reset_buffer(pool, buffer);
> > > +	GST_BUFFER_FLAGS(buffer) = 0;
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > > +{
> > > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > > +	gst_atomic_queue_push(self->queue, (gpointer)buffer);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_pool_init(GstLibcameraPool *self)
> > > +{
> > > +	self->queue = gst_atomic_queue_new(4);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_pool_finalize(GObject *object)
> > > +{
> > > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(object);
> > > +	GstBuffer *buf;
> > > +
> > > +	while ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))
> > > +		gst_buffer_unref(buf);
> > > +
> > > +	gst_atomic_queue_unref(self->queue);
> > > +	g_object_unref(self->allocator);
> > > +
> > > +	G_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);
> > > +}
> > > +
> > > +static void
> > > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> > > +{
> > > +	auto *object_class = G_OBJECT_CLASS(klass);
> > > +	auto *pool_class = GST_BUFFER_POOL_CLASS(klass);
> > > +
> > > +	object_class->finalize = gst_libcamera_pool_finalize;
> > > +	pool_class->start = nullptr;
> > > +	pool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;
> > > +	pool_class->reset_buffer = gst_libcamera_pool_reset_buffer;
> > > +	pool_class->release_buffer = gst_libcamera_pool_release_buffer;
> > > +}
> > > +
> > > +GstLibcameraPool *
> > > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> > > +{
> > > +	auto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);
> > > +
> > > +	pool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);
> > > +	pool->stream = stream;
> > > +
> > > +	gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> > > +	for (gsize i = 0; i < pool_size; i++) {
> > > +		GstBuffer *buffer = gst_buffer_new();
> > > +		gst_atomic_queue_push(pool->queue, buffer);
> > > +	}
> > > +
> > > +	return pool;
> > > +}
> > > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> > > new file mode 100644
> > > index 0000000..ca6b299
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcamerapool.h
> > > @@ -0,0 +1,27 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > + *
> > > + * gstlibcamerapool.h - GStreamer Buffer Pool
> > > + *
> > > + * This is a partial implementation of GstBufferPool intended for internal use
> > > + * only. This pool cannot be configured or activated.
> > > + */
> > > +
> > > +#include <gst/gst.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "gstlibcameraallocator.h"
> > > +
> > > +#ifndef __GST_LIBCAMERA_POOL_H__
> > > +#define __GST_LIBCAMERA_POOL_H__
> > > +
> > > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()
> > > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,
> > > +		     GST_LIBCAMERA, POOL, GstBufferPool)
> > > +
> > > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> > > +					 libcamera::Stream *stream);
> > > +
> > > +#endif /* __GST_LIBCAMERA_POOL_H__ */
> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > > index e497bf4..346910f 100644
> > > --- a/src/gstreamer/meson.build
> > > +++ b/src/gstreamer/meson.build
> > > @@ -4,6 +4,8 @@ libcamera_gst_sources = [
> > >      'gstlibcamerasrc.cpp',
> > >      'gstlibcameraprovider.cpp',
> > >      'gstlibcamerapad.cpp',
> > > +    'gstlibcameraallocator.cpp',
> > > +    'gstlibcamerapool.cpp'
> > >  ]
> > >  
> > >  libcamera_gst_c_args = [
> > > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [
> > >      '-DPACKAGE="@0@"'.format(meson.project_name()),
> > >  ]
> > >  
> > > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',
> > > +gst_req = '>=1.16.1'
> > 
> > Maybe gst_dep_version instead of gst_req ?
> > 
> > > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,
> > > +    required : get_option('gstreamer'))
> > > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,
> > >      required : get_option('gstreamer'))
> > >  
> > > -if gst_dep.found()
> > > +if gstvideo_dep.found() and gstallocator_dep.found()
> > >    libcamera_gst = shared_library('gstlibcamera',
> > >        libcamera_gst_sources,
> > >        c_args : libcamera_gst_c_args,
> > >        include_directories : [],
> > > -      dependencies : [libcamera_dep, gst_dep],
> > > +      dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],
> > >        install: true,
> > >        install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
> > >    )
>

Le mercredi 12 février 2020 à 02:16 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 28, 2020 at 10:32:04PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > This is needed to track the livetime of the FrameBufferAllocator in relation to
> 
> s/livetime/lifetime/
> 
> > the GstBuffer/GstMemory objects travelling inside GStreamer.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  src/gstreamer/gstlibcameraallocator.cpp | 240 ++++++++++++++++++++++++
> >  src/gstreamer/gstlibcameraallocator.h   |  29 +++
> >  src/gstreamer/gstlibcamerapool.cpp      | 109 +++++++++++
> >  src/gstreamer/gstlibcamerapool.h        |  27 +++
> >  src/gstreamer/meson.build               |  11 +-
> >  5 files changed, 413 insertions(+), 3 deletions(-)
> >  create mode 100644 src/gstreamer/gstlibcameraallocator.cpp
> >  create mode 100644 src/gstreamer/gstlibcameraallocator.h
> >  create mode 100644 src/gstreamer/gstlibcamerapool.cpp
> >  create mode 100644 src/gstreamer/gstlibcamerapool.h
> > 
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > new file mode 100644
> > index 0000000..0f248b4
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -0,0 +1,240 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcameraallocator.cpp - GStreamer Custom Allocator
> > + */
> > +
> > +#include "gstlibcameraallocator.h"
> > +#include "gstlibcamera-utils.h"
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/stream.h>
> > +#include <libcamera/framebuffer_allocator.h>
> 
> Could you please keep those headers alphabetically sorted ?
> 
> > +
> > +using namespace libcamera;
> > +
> > +/***********************************************************************/
> > +/* Internal object for tracking memories associated with a FrameBuffer */
> > +/***********************************************************************/
> 
> Same comment as before regarding this, a doxygen-style comment with a
> tiny bit of doc may be useful for inexperienced readers such as your
> devoted reviewer :-) You could combine the next comment with it.
> 
> > +
> > +static gboolean gst_libcamera_allocator_release(GstMiniObject *mini_object);
> 
> It would be nice to avoid forward declarations, but it may not be worth
> it.

It's always my goal, but this one was not avoidable.

> 
> > +
> > +/* This internal object is used to track the outstanding GstMemory object that
> 
> s/object/objects/
> 
> > + * are part of a FrameBuffer. The allocator will only re-use a FrameBuffer when
> > + * al all outstranging GstMemory have returned.
> 
> s/al all/all/ ?
> s/outstranging/outstanding/ ?
> 
> > + */
> > +struct FrameWrap {
> > +	FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > +		  gpointer stream);
> > +	~FrameWrap();
> > +
> > +	void AcquirePlane() { ++outstanding_planes_; }
> > +	bool ReleasePlane() { return --outstanding_planes_ == 0; }
> 
> Methods should start with a lowercase letter (camelCase, not CamelCase
> as for type names).
> 
> > +
> > +	gpointer stream_;
> > +	FrameBuffer *buffer_;
> > +	std::vector<GstMemory *> planes_;
> > +	gint outstanding_planes_;
> 
> outstandingPlanes_ as it's a C++ class ?
> 
> > +};
> > +
> > +static GQuark
> > +gst_libcamera_frame_quark(void)
> 
> How about making this a static function of the FrameWrap class ?
> 
> > +{
> > +	static gsize frame_quark = 0;
> > +
> > +	if (g_once_init_enter(&frame_quark)) {
> > +		GQuark quark = g_quark_from_string("GstLibCameraFrameWrap");
> > +		g_once_init_leave(&frame_quark, quark);
> > +	}
> > +
> > +	return frame_quark;
> > +}
> > +
> > +FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > +		     gpointer stream)
> > +
> > +	: stream_(stream),
> > +	  buffer_(buffer),
> > +	  outstanding_planes_(0)
> > +{
> > +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > +		GstMemory *mem = gst_fd_allocator_alloc(allocator,
> > +							plane.fd.fd(),
> > +							plane.length,
> > +							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> > +		gst_mini_object_set_qdata(GST_MINI_OBJECT(mem),
> > +					  gst_libcamera_frame_quark(),
> > +					  this, NULL);
> > +		GST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;
> > +		g_object_unref(mem->allocator);
> > +		planes_.push_back(mem);
> > +	}
> > +}
> > +
> > +FrameWrap::~FrameWrap()
> > +{
> > +	for (GstMemory *mem : planes_) {
> > +		GST_MINI_OBJECT(mem)->dispose = nullptr;
> > +		g_object_ref(mem->allocator);
> > +		gst_memory_unref(mem);
> > +	}
> > +}
> > +
> > +/***********************************/
> > +/* The GstAllocator implementation */
> > +/***********************************/
> > +struct _GstLibcameraAllocator {
> > +	GstDmaBufAllocator parent;
> > +	FrameBufferAllocator *fb_allocator;
> > +	/* A hash table using Stream pointer as key and returning a GQueue of
> > +	 * FrameWrap. */
> > +	GHashTable *pools;
> > +};
> > +
> > +G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > +	      GST_TYPE_DMABUF_ALLOCATOR);
> > +
> > +static gboolean
> > +gst_libcamera_allocator_release(GstMiniObject *mini_object)
> > +{
> > +	GstMemory *mem = GST_MEMORY_CAST(mini_object);
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);
> > +	GST_OBJECT_LOCKER(self);
> > +	gpointer data = gst_mini_object_get_qdata(mini_object,
> > +						  gst_libcamera_frame_quark());
> > +	auto *frame = (FrameWrap *)data;
> 
> Could you use reinterpret_cast<FrameWrap *>(data) here ? And maybe
> replace auto with FrameWrap as it's short.
> 
> In general, please use static_cast or reinterpret_cast as appropriate,
> as they offer more compile-time protection that C-style casts. I won't
> comment on other occurrences.
> 
> > +
> > +	gst_memory_ref(mem);
> > +
> > +	if (frame->ReleasePlane()) {
> > +		auto *pool = (GQueue *)g_hash_table_lookup(self->pools, frame->stream_);
> > +		g_return_val_if_fail(pool, TRUE);
> > +		g_queue_push_tail(pool, frame);
> > +	}
> > +
> > +	/* Keep last in case we are holding on the last allocator ref */
> > +	g_object_unref(mem->allocator);
> > +
> > +	/* Rreturns FALSE so that our mini object isn't freed */
> 
> s/Rreturns/Return/
> 
> > +	return FALSE;
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_free_pool(gpointer data)
> > +{
> > +	GQueue *queue = (GQueue *)data;
> > +	FrameWrap *frame;
> > +
> > +	while ((frame = (FrameWrap *)g_queue_pop_head(queue))) {
> > +		g_warn_if_fail(frame->outstanding_planes_ == 0);
> > +		delete frame;
> > +	}
> > +
> > +	g_queue_free(queue);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_init(GstLibcameraAllocator *self)
> > +{
> > +	self->pools = g_hash_table_new_full(NULL, NULL, NULL,
> > +					    gst_libcamera_allocator_free_pool);
> > +	GST_OBJECT_FLAG_SET(self, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_dispose(GObject *object)
> > +{
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > +
> > +	if (self->pools) {
> > +		g_hash_table_unref(self->pools);
> > +		self->pools = NULL;
> > +	}
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->dispose(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_finalize(GObject *object)
> > +{
> > +	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(object);
> > +
> > +	delete self->fb_allocator;
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)
> > +{
> > +	auto *allocator_class = GST_ALLOCATOR_CLASS(klass);
> > +	auto *object_class = G_OBJECT_CLASS(klass);
> > +
> > +	object_class->dispose = gst_libcamera_allocator_dispose;
> > +	object_class->finalize = gst_libcamera_allocator_finalize;
> > +	allocator_class->alloc = NULL;
> > +}
> > +
> > +GstLibcameraAllocator *
> > +gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
> > +{
> > +	auto *self = (GstLibcameraAllocator *)g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > +							   nullptr);
> > +
> > +	self->fb_allocator = FrameBufferAllocator::create(camera);
> > +	for (Stream *stream : camera->streams()) {
> > +		gint ret;
> > +
> > +		ret = self->fb_allocator->allocate(stream);
> > +		if (ret == 0)
> > +			return nullptr;
> > +
> > +		GQueue *pool = g_queue_new();
> > +		for (const std::unique_ptr<FrameBuffer> &buffer :
> > +		     self->fb_allocator->buffers(stream)) {
> > +			auto *fb = new FrameWrap(GST_ALLOCATOR(self),
> > +						 buffer.get(), stream);
> > +			g_queue_push_tail(pool, fb);
> > +		}
> > +
> > +		g_hash_table_insert(self->pools, stream, pool);
> > +	}
> > +
> > +	return self;
> > +}
> > +
> > +bool
> > +gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > +				       Stream *stream, GstBuffer *buffer)
> > +{
> > +	GST_OBJECT_LOCKER(self);
> > +
> > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > +	g_return_val_if_fail(pool, false);
> > +
> > +	auto *frame = (FrameWrap *)g_queue_pop_head(pool);
> > +	if (!frame)
> > +		return false;
> > +
> > +	for (GstMemory *mem : frame->planes_) {
> > +		frame->AcquirePlane();
> > +		gst_buffer_append_memory(buffer, mem);
> > +		g_object_ref(mem->allocator);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +gsize
> > +gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,
> > +				      Stream *stream)
> > +{
> > +	GST_OBJECT_LOCKER(self);
> > +
> > +	auto *pool = (GQueue *)g_hash_table_lookup(self->pools, stream);
> > +	g_return_val_if_fail(pool, false);
> > +
> > +	return pool->length;
> > +}
> > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h
> > new file mode 100644
> > index 0000000..f2a0f58
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraallocator.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcameraallocator.h - GStreamer Custom Allocator
> > + */
> > +
> > +#include <gst/gst.h>
> > +#include <gst/allocators/allocators.h>
> > +#include <libcamera/stream.h>
> > +
> > +#ifndef __GST_LIBCAMERA_ALLOCATOR_H__
> > +#define __GST_LIBCAMERA_ALLOCATOR_H__
> > +
> > +#define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > +		     GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)
> > +
> > +GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera);
> > +
> > +bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > +					    libcamera::Stream *stream,
> > +					    GstBuffer *buffer);
> > +
> > +gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,
> > +					    libcamera::Stream *stream);
> > +
> > +#endif /* __GST_LIBCAMERA_ALLOCATOR_H__ */
> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> > new file mode 100644
> > index 0000000..f84d1d6
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcamerapool.cpp
> > @@ -0,0 +1,109 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcamerapool.cpp - GStreamer Buffer Pool
> > + */
> > +
> > +#include "gstlibcamerapool.h"
> > +#include "gstlibcamera-utils.h"
> > +
> > +#include <libcamera/stream.h>
> > +
> > +using namespace libcamera;
> > +
> > +struct _GstLibcameraPool {
> > +	GstBufferPool parent;
> > +
> > +	GstAtomicQueue *queue;
> > +	GstLibcameraAllocator *allocator;
> > +	Stream *stream;
> > +};
> > +
> > +G_DEFINE_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_TYPE_BUFFER_POOL);
> > +
> > +static GstFlowReturn
> > +gst_libcamera_pool_acquire_buffer(GstBufferPool *pool, GstBuffer **buffer,
> > +				  GstBufferPoolAcquireParams *params)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > +	GstBuffer *buf = (GstBuffer *)gst_atomic_queue_pop(self->queue);
> > +	if (!buf)
> > +		return GST_FLOW_ERROR;
> > +
> > +	if (!gst_libcamera_allocator_prepare_buffer(self->allocator, self->stream, buf))
> > +		return GST_FLOW_ERROR;
> > +
> > +	*buffer = buf;
> > +	return GST_FLOW_OK;
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_reset_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > +{
> > +	GstBufferPoolClass *klass = GST_BUFFER_POOL_CLASS(gst_libcamera_pool_parent_class);
> > +
> > +	/* Clears all the memories and only pool the GstBuffer objects */
> > +	gst_buffer_remove_all_memory(buffer);
> > +	klass->reset_buffer(pool, buffer);
> > +	GST_BUFFER_FLAGS(buffer) = 0;
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> > +	gst_atomic_queue_push(self->queue, (gpointer)buffer);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_init(GstLibcameraPool *self)
> > +{
> > +	self->queue = gst_atomic_queue_new(4);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_finalize(GObject *object)
> > +{
> > +	GstLibcameraPool *self = GST_LIBCAMERA_POOL(object);
> > +	GstBuffer *buf;
> > +
> > +	while ((buf = (GstBuffer *)gst_atomic_queue_pop(self->queue)))
> > +		gst_buffer_unref(buf);
> > +
> > +	gst_atomic_queue_unref(self->queue);
> > +	g_object_unref(self->allocator);
> > +
> > +	G_OBJECT_CLASS(gst_libcamera_pool_parent_class)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
> > +{
> > +	auto *object_class = G_OBJECT_CLASS(klass);
> > +	auto *pool_class = GST_BUFFER_POOL_CLASS(klass);
> > +
> > +	object_class->finalize = gst_libcamera_pool_finalize;
> > +	pool_class->start = nullptr;
> > +	pool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;
> > +	pool_class->reset_buffer = gst_libcamera_pool_reset_buffer;
> > +	pool_class->release_buffer = gst_libcamera_pool_release_buffer;
> > +}
> > +
> > +GstLibcameraPool *
> > +gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
> > +{
> > +	auto *pool = (GstLibcameraPool *)g_object_new(GST_TYPE_LIBCAMERA_POOL, nullptr);
> > +
> > +	pool->allocator = (GstLibcameraAllocator *)g_object_ref(allocator);
> > +	pool->stream = stream;
> > +
> > +	gsize pool_size = gst_libcamera_allocator_get_pool_size(allocator, stream);
> > +	for (gsize i = 0; i < pool_size; i++) {
> > +		GstBuffer *buffer = gst_buffer_new();
> > +		gst_atomic_queue_push(pool->queue, buffer);
> > +	}
> > +
> > +	return pool;
> > +}
> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> > new file mode 100644
> > index 0000000..ca6b299
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcamerapool.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > + *
> > + * gstlibcamerapool.h - GStreamer Buffer Pool
> > + *
> > + * This is a partial implementation of GstBufferPool intended for internal use
> > + * only. This pool cannot be configured or activated.
> > + */
> > +
> > +#include <gst/gst.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "gstlibcameraallocator.h"
> > +
> > +#ifndef __GST_LIBCAMERA_POOL_H__
> > +#define __GST_LIBCAMERA_POOL_H__
> > +
> > +#define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool,
> > +		     GST_LIBCAMERA, POOL, GstBufferPool)
> > +
> > +GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
> > +					 libcamera::Stream *stream);
> > +
> > +#endif /* __GST_LIBCAMERA_POOL_H__ */
> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > index e497bf4..346910f 100644
> > --- a/src/gstreamer/meson.build
> > +++ b/src/gstreamer/meson.build
> > @@ -4,6 +4,8 @@ libcamera_gst_sources = [
> >      'gstlibcamerasrc.cpp',
> >      'gstlibcameraprovider.cpp',
> >      'gstlibcamerapad.cpp',
> > +    'gstlibcameraallocator.cpp',
> > +    'gstlibcamerapool.cpp'
> >  ]
> >  
> >  libcamera_gst_c_args = [
> > @@ -11,15 +13,18 @@ libcamera_gst_c_args = [
> >      '-DPACKAGE="@0@"'.format(meson.project_name()),
> >  ]
> >  
> > -gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',
> > +gst_req = '>=1.16.1'
> 
> Maybe gst_dep_version instead of gst_req ?
> 
> > +gstvideo_dep = dependency('gstreamer-video-1.0', version : gst_req,
> > +    required : get_option('gstreamer'))
> > +gstallocator_dep = dependency('gstreamer-allocators-1.0', version : gst_req,
> >      required : get_option('gstreamer'))
> >  
> > -if gst_dep.found()
> > +if gstvideo_dep.found() and gstallocator_dep.found()
> >    libcamera_gst = shared_library('gstlibcamera',
> >        libcamera_gst_sources,
> >        c_args : libcamera_gst_c_args,
> >        include_directories : [],
> > -      dependencies : [libcamera_dep, gst_dep],
> > +      dependencies : [libcamera_dep, gstvideo_dep, gstallocator_dep],
> >        install: true,
> >        install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
> >    )