[v2,1/3] gstreamer: allocator: Ensure camera manager stay alive
diff mbox series

Message ID 20240305153058.1761020-2-nicolas@ndufresne.ca
State Accepted
Headers show
Series
  • gstreamer: Fix a crash when memory outlives the pipeline
Related show

Commit Message

Nicolas Dufresne March 5, 2024, 3:30 p.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

With the camera manager, it is not possible to cleanly delete the
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@collabora.com>
---
 src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Kieran Bingham May 9, 2024, 2:44 p.m. UTC | #1 
Quoting Nicolas Dufresne (2024-03-05 15:30:56)
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> With the camera manager, it is not possible to cleanly delete the

Does this mean 'with' or 'without the camera manager' ?

> FrameBufferAllocator object. Keep the camera manager alive until all the
> memory object have been released.
> 
> Fixes Bugzilla issue 211
> 

Bug: https://bugs.libcamera.org/show_bug.cgi?id=211

> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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.

s/represent that/represents the/ ?
s/have/has/

But those are just nits could fixed while applying.

I recall Laurent saying he had some worries about this series, thinking
that there might be a better way to handle keeping the lifetime managed.

But - This fixes a bug, and is fully green on the CI, and even
introduces a test to validate it. So I'd be keen to see this series
merged. There's always the opportunity to refactor in the future if
someone desired it.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +        */
> +       std::shared_ptr<CameraManager> *cm_ptr;
>  };
>  
>  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;
> +
>         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));
> +       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)
> -- 
> 2.43.2
>
Nicolas Dufresne May 9, 2024, 3:05 p.m. UTC | #2 
Hi,

thanks for the review,

Le jeudi 09 mai 2024 à 15:44 +0100, Kieran Bingham a écrit :
> Quoting Nicolas Dufresne (2024-03-05 15:30:56)
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > With the camera manager, it is not possible to cleanly delete the
> 
> Does this mean 'with' or 'without the camera manager' ?

Without, sorry.

> 
> > FrameBufferAllocator object. Keep the camera manager alive until all the
> > memory object have been released.
> > 
> > Fixes Bugzilla issue 211
> > 
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=211

Ack. (did we document our tagging somewhere ?)

> 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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.
> 
> s/represent that/represents the/ ?
> s/have/has/
> 
> But those are just nits could fixed while applying.

Ack.

> 
> I recall Laurent saying he had some worries about this series, thinking
> that there might be a better way to handle keeping the lifetime managed.

Sure, though my opinion is that project that ships should accept valid work
around for serious issues that will requires month to be fixed properly.

> 
> But - This fixes a bug, and is fully green on the CI, and even
> introduces a test to validate it. So I'd be keen to see this series
> merged. There's always the opportunity to refactor in the future if
> someone desired it.

Indeed, the idea with the test, is that you can later revert this patch but keep
the test. Since the test will be green if you figure-out another way to fix it.
Perhaps it could have been a separate patch for making this easier. I simply
didn't think about that earlier.

Let me know, I can resend if need be.

> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +        */
> > +       std::shared_ptr<CameraManager> *cm_ptr;
> >  };
> >  
> >  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;
> > +
> >         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));
> > +       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)
> > -- 
> > 2.43.2
> >
Laurent Pinchart May 14, 2024, 4:08 p.m. UTC | #3 
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@collabora.com>
> 
> With the camera manager, it is not possible to cleanly delete the

Did you mean s/With/Without/ ?

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

	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)
Nicolas Dufresne May 14, 2024, 4:53 p.m. UTC | #4 
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@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@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)
>
Laurent Pinchart May 14, 2024, 6:40 p.m. UTC | #5 
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@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@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)
Laurent Pinchart May 14, 2024, 8:25 p.m. UTC | #6 
On Thu, May 09, 2024 at 03:44:19PM +0100, Kieran Bingham wrote:
> Quoting Nicolas Dufresne (2024-03-05 15:30:56)
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > With the camera manager, it is not possible to cleanly delete the
> 
> Does this mean 'with' or 'without the camera manager' ?
> 
> > FrameBufferAllocator object. Keep the camera manager alive until all the
> > memory object have been released.
> > 
> > Fixes Bugzilla issue 211
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=211
> 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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.
> 
> s/represent that/represents the/ ?
> s/have/has/
> 
> But those are just nits could fixed while applying.
> 
> I recall Laurent saying he had some worries about this series, thinking
> that there might be a better way to handle keeping the lifetime managed.

I (finally) had a look at that today, and I think this patch is the way
to go, at least for now. We should redesign the FrameBufferAllocator at
some point, the current API isn't great, but for the time being, this is
the best way forward.

> But - This fixes a bug, and is fully green on the CI, and even
> introduces a test to validate it. So I'd be keen to see this series
> merged. There's always the opportunity to refactor in the future if
> someone desired it.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > +        */
> > +       std::shared_ptr<CameraManager> *cm_ptr;
> >  };
> >  
> >  G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)
> >  
> >         delete self->fb_allocator;
> >  
> > +       /* keep last */

s/keep last/Keep last./

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +       delete self->cm_ptr;
> > +
> >         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));
> > +       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)

Patch
diff mbox series 

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;
 };
 
 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;
+
 	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));
+	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)