[PATCH v2 3/3] test: gstreamer: Test memory lifetime
Nicolas Dufresne
nicolas.dufresne at collabora.com
Mon May 13 17:18:01 CEST 2024
Hi,
Le lundi 13 mai 2024 à 01:35 +0300, Laurent Pinchart a écrit :
> On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:
> > Quoting Nicolas Dufresne (2024-05-09 16:28:14)
> > > 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.
> >
> > Yes, startPipeline is obvious enough ;-)
> >
> > > > > +
> > > > > + 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.
> >
> > Agreed, but what's the impact in lifetime. do we expect a single buffer?
> > I think you're covering it on the comment below though.
> >
> > > > > + 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.
> > > */
> >
> > Yes, that's what I think the test needs to document.
>
> +1. That's also the part I wasn't sure about.
>
> > > > > +
> > > > > + 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.
>
> Should/can it be checked before calling gst_element_set_state(), or is
> the order above important ? sample_ being a normal pointer I assume that
> the call to gst_element_set_state() won't modify it, but the order of
> the code made it feel like there was some magic happening.
If you prefer, we can duplicate the set_state call, here's the pseudo code of
what can be done, also added a draft of the missing comment.
sample = pull()
if (!sample) {
set_state(NULL); return failed;
} set_state(NULL);
/*
* Keep the sample referenced. This way, it will be released
* in cleanup() after the camera manager object. This used to
* lead to a crash as freeing video frames tried to call into
* the freed camera manager.
*/
return success;
The reason being that behaviour is undefined (and document to deadlock most of
the time) if you drop the last reference of a pipeline that is running (any
state above NULL).
>
> > Which is the point that spots the code is broken then? I seem to still
> > miss it ?
> >
> > > > 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.
>
> Sorry for not replying earlier.
>
> > Sounds like a great reason to make sure we have comments in the code
> > that explain the 'why' :-)
> >
> > I'm fine with whatever comments you believe are appropriate. 'You' two
> > months later are probably the best reviewer here ;-)
>
> Another comment about this patch in particular. We tend to order this
> kind of series with the test coming first, and the fix coming next. This
> allows ensuring that the test fails, and that the fix fixes the issue.
> This guards against cases where the test would be incorrect and wouldn't
> showcase the actual problem, and the fix would also be bogus.
Sure, as we discussed on IRC, in my other projects we do the total opposite to
avoid making bisection painful. Might be something to revisit later for you.
Nicolas
>
> > > > > +
> > > > > + return TestPass;
> > > > > + }
> > > > > +
> > > > > + void cleanup() override
> > > > > + {
> >
> > Are you saying the test is actually testing the issue /here/ ? If so -
> > can we add a comment explicitly saying that ?
> >
> > Otherwise this is just 'cleanup' code to me. That's why I was looking at
> > the null check of sample_ trying to figure out if that's how you were
> > determining if the test had failed (Becuase that's the only time you
> > return TestFail after the test actually starts).
> >
> >
> > I guess the failure point is that the test will crash, rather than
> > detect a failure. Would that also be noteworthy?
>
> That is worth a comment, yes.
>
> > > > > + 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)
> > > > >
>
More information about the libcamera-devel
mailing list