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

[Nicolas Dufresne]

Hi Laurent,

Le mardi 14 mai 2024 à 19:08 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> 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.

regards,
Nicolas

> 
> 	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)
>