| Message ID | 20240305153058.1761020-4-nicolas@ndufresne.ca |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Nicolas Dufresne (2024-03-05 15:30:58) > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Test that everything works fine if a buffer outlives the pipeline. > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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; > + > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5); > + gst_element_set_state(pipeline_, GST_STATE_NULL); > + > + if (!sample_) > + return TestFail; Is this the part that does the test? I don't really know how this test is working. Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy the pipeline? 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. 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. > + > + 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 >
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@collabora.com> > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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 > >
Quoting Nicolas Dufresne (2024-05-09 16:28:14) > 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@collabora.com> > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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. > > > > + > > > + 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. 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. 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 ;-) > > 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? > > > + 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 > > > >
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@collabora.com> > > > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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. > 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. > > > > + > > > > + 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) > > > >
On Mon, May 13, 2024 at 01:35:04AM +0300, Laurent Pinchart wrote: > 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@collabora.com> > > > > > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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. > > > 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. I confirm that this patch, without patch 1/3, causes a crash when libcamera is compiled without ASan, and an ASan failure otherwise. Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + > > > > > + 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) > > > > >
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@collabora.com> > > > > > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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) > > > > > >
Hi Nicolas, On Mon, May 13, 2024 at 11:18:01AM -0400, Nicolas Dufresne wrote: > 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@collabora.com> > > > > > > > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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; Normally I would go for the code included in this patch, it's more efficient. In this specific case, as the code is a test case, I think it's important to make it as clear as possible. How about the following (which verifies my good understanding of the test case) ? sample = pull() if (!sample) { set_state(NULL); return failed; } /* * Keep the sample referenced and set the pipeline state to NULL. This * causes the libcamerasrc element to synchronously release resources it * holds. The sample will be released later in cleanup(). * * The test case verifies that libcamerasrc keeps alive long enough all * the resources that are needed until memory allocated for frames gets * freed. Any use-after-free will be caught by the test crashing in * cleanup(). */ set_state(NULL); 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. Sure. We could also mark the test as expected to fail, and turn it into expected to succeed in the patch that fixes the issue. > > > > > > + > > > > > > + 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) > > > > > >
Quoting Laurent Pinchart (2024-05-14 17:02:16) > Hi Nicolas, > > On Mon, May 13, 2024 at 11:18:01AM -0400, Nicolas Dufresne wrote: > > 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@collabora.com> > > > > > > > > > > > > > > Test that everything works fine if a buffer outlives the pipeline. > > > > > > > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@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; > > Normally I would go for the code included in this patch, it's more > efficient. In this specific case, as the code is a test case, I think > it's important to make it as clear as possible. How about the following > (which verifies my good understanding of the test case) ? > > sample = pull() > if (!sample) { > set_state(NULL); > return failed; > } > > /* > * Keep the sample referenced and set the pipeline state to NULL. This > * causes the libcamerasrc element to synchronously release resources it > * holds. The sample will be released later in cleanup(). > * > * The test case verifies that libcamerasrc keeps alive long enough all > * the resources that are needed until memory allocated for frames gets > * freed. Any use-after-free will be caught by the test crashing in > * cleanup(). > */ > set_state(NULL); > return success; > I think this is the best option to clarify the intent of the test, and document what's happening. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I'll make these changes while applying to get this series merged already, and also fix the test to run first with ExpectedFail which the commit that handles the fix will remove. -- Kieran > > 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. > > Sure. We could also mark the test as expected to fail, and turn it into > expected to succeed in the patch that fixes the issue. > > > > > > > > + > > > > > > > + 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) > > > > > > > > > -- > Regards, > > Laurent Pinchart
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; + + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5); + gst_element_set_state(pipeline_, GST_STATE_NULL); + + if (!sample_) + return TestFail; + + 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)