[PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay alive

[Laurent Pinchart]

Hi Nicolas,

On Tue, May 14, 2024 at 12:53:51PM -0400, Nicolas Dufresne wrote:
> Le mardi 14 mai 2024 à 19:08 +0300, Laurent Pinchart a écrit :
> > On Tue, Mar 05, 2024 at 10:30:56AM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > 
> > > With the camera manager, it is not possible to cleanly delete the
> > 
> > Did you mean s/With/Without/ ?
> 
> Without, was already mentioned before.
> 
> > > FrameBufferAllocator object. Keep the camera manager alive until all the
> > > memory object have been released.
> > > 
> > > Fixes Bugzilla issue 211
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > ---
> > >  src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > > index c740b8fc..844bdb17 100644
> > > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > > @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {
> > >  	 * FrameWrap.
> > >  	 */
> > >  	GHashTable *pools;
> > > +	/*
> > > +	 * The camera manager represent that library, which needs to be kept alive
> > > +	 * until all the memory have been released.
> > > +	 */
> > > +	std::shared_ptr<CameraManager> *cm_ptr;
> > 
> > This doesn't have to be a pointer, it can be
> 
> No, it can't, that C structure is alloc/freed by glib, as a side effect will be
> missing a destructor call to drop the shared pointer reference. In the
> GstElement code, that is why we have a C++ class, the "state". It makes
> everything more convenient. But until we have more then once C++ data member its
> not worth it.

You're right. Could you record this in the commit message ?

> > 	std::shared_ptr<CameraManager> cm;
> > 
> > >  };
> > >  
> > >  G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)
> > >  
> > >  	delete self->fb_allocator;
> > >  
> > > +	/* keep last */
> > > +	delete self->cm_ptr;
> > > +
> > 
> > This would become
> > 
> > 	/* Keep last. */
> > 	self->cm.reset();
> > 
> > >  	G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);
> > >  }
> > >  
> > > @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> > >  {
> > >  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > >  							  nullptr));
> > > +	gint ret;
> > > +
> > > +	self->cm_ptr = new std::shared_ptr<CameraManager>(gst_libcamera_get_camera_manager(ret));
> > 
> > And here,
> > 
> > 	self->cm = gst_libcamera_get_camera_manager(ret);
> > 
> > > +	if (ret) {
> > > +		g_object_unref(self);
> > > +		return nullptr;
> > > +	}
> > >  
> > >  	self->fb_allocator = new FrameBufferAllocator(camera);
> > >  	for (StreamConfiguration &streamCfg : *config_) {
> > >  		Stream *stream = streamCfg.stream();
> > > -		gint ret;
> > >  
> > >  		ret = self->fb_allocator->allocate(stream);
> > >  		if (ret == 0)

-- 
Regards,

Laurent Pinchart