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

[Nicolas Dufresne]

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

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 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 */
> > +       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
> >