[PATCH v2 3/3] test: gstreamer: Test memory lifetime

[Nicolas Dufresne]

Hi,

Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :
> Quoting Nicolas Dufresne (2024-03-05 15:30:58)
> > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > 
> > Test that everything works fine if a buffer outlives the pipeline.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  .../gstreamer_memory_lifetime_test.cpp        | 75 +++++++++++++++++++
> >  test/gstreamer/meson.build                    |  4 +-
> >  2 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp
> > 
> > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp
> > new file mode 100644
> > index 00000000..6d932e9e
> > --- /dev/null
> > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp
> > @@ -0,0 +1,75 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2024, Nicolas Dufresne
> > + *
> > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test
> > + */
> > +
> > +#include <iostream>
> > +#include <unistd.h>
> > +
> > +#include <gst/app/app.h>
> > +#include <gst/gst.h>
> > +
> > +#include "gstreamer_test.h"
> > +#include "test.h"
> > +
> > +using namespace std;
> > +
> > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test
> > +{
> > +public:
> > +       GstreamerMemoryLifetimeTest()
> > +               : GstreamerTest()
> > +       {
> > +       }
> > +
> > +protected:
> > +       int init() override
> > +       {
> > +               if (status_ != TestPass)
> > +                       return status_;
> > +
> > +               appsink_ = gst_element_factory_make("appsink", nullptr);
> > +               if (!appsink_) {
> > +                       g_printerr("Your installation is missing 'appsink'\n");
> > +                       return TestFail;
> > +               }
> > +               g_object_ref_sink(appsink_);
> > +
> > +               return createPipeline();
> > +       }
> > +
> > +       int run() override
> > +       {
> > +               /* Build the pipeline */
> > +               gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);
> > +               if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {
> > +                       g_printerr("Elements could not be linked.\n");
> > +                       return TestFail;
> > +               }
> > +
> > +               if (startPipeline() != TestPass)
> > +                       return TestFail;

Did not add a comment here, since "startPipeline()" should be obvious that we
start the pipeline. It will raise the state to PLAYING state.

> > +
> > +               sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);

This is also a bit trivial, it pulls a sample from the pipeline.

> > +               gst_element_set_state(pipeline_, GST_STATE_NULL);

Here though, I should probably add a comment (well before the change to NULL
state).

/* 
 * Stop the pipeline without releasing sample_. This ensures that we can hold
 * on memory while elements can release all their internal resources as required
 * by the NULL state definition.
 */

> > +
> > +               if (!sample_)
> > +                       return TestFail;
> 
> Is this the part that does the test? I don't really know how this test
> is working.

This is memory safety. As the doc says:

https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample

>  Returns ( [transfer: full][nullable]) – a GstSample or NULL ...

As it annoted nullable, not doing a null check if not safe. I'd, say, it would
be annormal, so failing the test make sense, but its not really the expected
failure spot for when the code was broken.

> 
> Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy
> the pipeline?

No, when moving to STATE_NULL, its is required 

> 
> Any comments we can add here to make it clearer what is actually being
> tested or happening? The code doesn't really tell me what's going on
> under the hood.

https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states

> GST_STATE_NULL: this is the default state. No resources are allocated in this
state, so, transitioning to it will free all resources. The element must be in 
this state when its refcount reaches 0 and it is freed.

Let me know if the suggested comment above would do for you.

> 
> Does the test for !sample_ really mean the buffer still exists and is
> valid? Does the test need to do anything to /access/ the buffer to make
> sure the test proves it succeeded?
> 
> I expect it probably does as otherwise you'd have added more - but it's
> just not obvious from the above.
> 
> 

The bug being that libcamera crash when the memory is released, in this specific
case it crash in cleanup(). But I suspect that we can drop the sample_ member,
and simple manually unref the sample here (avoiding smart pointer, as it would
be equally confusing).

p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I
don't remember much about it, I'm just reading the patch to reply here.

Nicolas

> 
> > +
> > +               return TestPass;
> > +       }
> > +
> > +       void cleanup() override
> > +       {
> > +               g_clear_pointer(&sample_, gst_sample_unref);
> > +               g_clear_object(&appsink_);
> > +       }
> > +
> > +private:
> > +       GstElement *appsink_;
> > +       GstSample *sample_;
> > +};
> > +
> > +TEST_REGISTER(GstreamerMemoryLifetimeTest)
> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build
> > index f3ba5a23..70609b88 100644
> > --- a/test/gstreamer/meson.build
> > +++ b/test/gstreamer/meson.build
> > @@ -8,12 +8,14 @@ gstreamer_tests = [
> >      {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},
> >      {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},
> >      {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},
> > +    {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},
> >  ]
> >  gstreamer_dep = dependency('gstreamer-1.0', required : true)
> > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)
> >  
> >  foreach test : gstreamer_tests
> >      exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',
> > -                     dependencies : [libcamera_private, gstreamer_dep],
> > +                     dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> >  
> > -- 
> > 2.43.2
> >