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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart