[libcamera-devel] [PATCH v1] test: gstreamer: Fix the destructor of GstreamerTest base class

[Kieran Bingham]

On 08/09/2021 14:20, Vedant Paranjape wrote:
> Hi Kieran,
> 
> On Wed, Sep 8, 2021 at 6:36 PM Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> wrote:
> 
>     On 08/09/2021 13:04, Vedant Paranjape wrote:
>     > The destructor tried to check if pipeline_ is a parent of
>     libcameraSrc_.
>     > This was needed to be checked as if it is, cleanup of libcameraSrc_
>     > would be handled by pipeline itself.
>     >
>     > Since, the destructor can be called anytime, even when pipeline_
>     hasn't
>     > been created, the use of pipeline_ to check if libcameraSrc_ has an
>     > ancestor as pipeline_ caused a segmentation fault.
> 
>     I presume the refcounting is what actually deals with the cleanup
>     accordingly now?
> 
> 
> Right !
> 
>     Will the gst_object_unref(pipeline_); have any effect such as making
>     libcameraSrc_ invalid? (because you said that the pipeline handler would
> 
> 
> Yes, it will unref the libcameraSrc_, and I think gst_deinit will free
> it. Pinging @Nicolas Dufresne <mailto:nicolas at ndufresne.ca> for clearity.

The question is - have you made sure that the refcounting is correctly
balanced, such that this final gst_object_unref(libcameraSrc_);  will be
the one that does the release?

I.e. to make sure it doesn't take the refcount below zero.


> 
>     It looks reasonable otherwise:
> 
>     Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com
>     <mailto:kieran.bingham at ideasonboard.com>>
> 
>     >
>     > Fixes: f58768092277 ("test: gstreamer: Fix the destructor of
>     GstreamerTest base class")
>     > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com
>     <mailto:vedantparanjape160201 at gmail.com>>
>     > ---
>     >  test/gstreamer/gstreamer_test.cpp | 6 ++----
>     >  1 file changed, 2 insertions(+), 4 deletions(-)
>     >
>     > diff --git a/test/gstreamer/gstreamer_test.cpp
>     b/test/gstreamer/gstreamer_test.cpp
>     > index dbdcaec0b111..46fa5abaea75 100644
>     > --- a/test/gstreamer/gstreamer_test.cpp
>     > +++ b/test/gstreamer/gstreamer_test.cpp
>     > @@ -69,12 +69,10 @@ GstreamerTest::GstreamerTest()
>     > 
>     >  GstreamerTest::~GstreamerTest()
>     >  {
>     > -     if (libcameraSrc_ &&
>     > -         !gst_object_has_as_ancestor(GST_OBJECT(libcameraSrc_),
>     > -                                     GST_OBJECT(pipeline_)))
>     > -             gst_object_unref(libcameraSrc_);
>     >       if (pipeline_)
>     >               gst_object_unref(pipeline_);
>     > +     if (libcameraSrc_)
>     > +             gst_object_unref(libcameraSrc_);
>     > 
>     >       gst_deinit();
>     >  }
>     >
> 
> 
> Regards,
> /*Vedant Paranjape*/