[{"id":29504,"web_url":"https://patchwork.libcamera.org/comment/29504/","msgid":"<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>","date":"2024-05-09T14:57:25","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> From: Nicolas Dufresne \n> \n> Test that everything works fine if a buffer outlives the pipeline.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> test/gstreamer/meson.build | 4 +-\n> 2 files changed, 78 insertions(+), 1 deletion(-)\n> create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> \n> diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> new file mode 100644\n> index 00000000..6d932e9e\n> --- /dev/null\n> +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Nicolas Dufresne\n> + *\n> + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"gstreamer_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> +{\n> +public:\n> + GstreamerMemoryLifetimeTest()\n> + : GstreamerTest()\n> + {\n> + }\n> +\n> +protected:\n> + int init() override\n> + {\n> + if (status_ != TestPass)\n> + return status_;\n> +\n> + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> + if (!appsink_) {\n> + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> + return TestFail;\n> + }\n> + g_object_ref_sink(appsink_);\n> +\n> + return createPipeline();\n> + }\n> +\n> + int run() override\n> + {\n> + /* Build the pipeline */\n> + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> + g_printerr(\"Elements could not be linked.\\n\");\n> + return TestFail;\n> + }\n> +\n> + if (startPipeline() != TestPass)\n> + return TestFail;\n> +\n> + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> +\n> + if (!sample_)\n> + return TestFail;\n\nIs this the part that does the test? I don't really know how this test\nis working.\n\nIs it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\nthe pipeline?\n\nAny comments we can add here to make it clearer what is actually being\ntested or happening? The code doesn't really tell me what's going on\nunder the hood.\n\nDoes the test for !sample_ really mean the buffer still exists and is\nvalid? Does the test need to do anything to /access/ the buffer to make\nsure the test proves it succeeded?\n\nI expect it probably does as otherwise you'd have added more - but it's\njust not obvious from the above.\n\n\n\n> +\n> + return TestPass;\n> + }\n> +\n> + void cleanup() override\n> + {\n> + g_clear_pointer(&sample_, gst_sample_unref);\n> + g_clear_object(&appsink_);\n> + }\n> +\n> +private:\n> + GstElement *appsink_;\n> + GstSample *sample_;\n> +};\n> +\n> +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> index f3ba5a23..70609b88 100644\n> --- a/test/gstreamer/meson.build\n> +++ b/test/gstreamer/meson.build\n> @@ -8,12 +8,14 @@ gstreamer_tests = [\n> {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> ]\n> gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> \n> foreach test : gstreamer_tests\n> exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> - dependencies : [libcamera_private, gstreamer_dep],\n> + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> link_with : test_libraries,\n> include_directories : test_includes_internal)\n> \n> -- \n> 2.43.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 397AFC3226\n\tfor ;\n\tThu, 9 May 2024 14:57:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5536F633FA;\n\tThu, 9 May 2024 16:57:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D333633FA\n\tfor ;\n\tThu, 9 May 2024 16:57:28 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0AB13D5;\n\tThu, 9 May 2024 16:57:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g7FmFFmq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715266644;\n\tbh=yt61yVKusNNxAAWg9ssPwU47IOJ7H0pLEuPwmulWR6w=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=g7FmFFmqqTkKV9bkztC19g/G6ZPRkI50LgjEcDb7ONoRl2BOCiscqBdOSMmvbECyE\n\toOv2OrlapvFIH0ecwWhAut0ZGTWlRAcdjIBnLa4CCcF6misw3lno8u01aai1bvitpt\n\tBM8QRBFxzL2pRMTS4Wfz+ycI9wBGlbCxOfB3IXuU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240305153058.1761020-4-nicolas@ndufresne.ca>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","From":"Kieran Bingham ","Cc":"Nicolas Dufresne ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 May 2024 15:57:25 +0100","Message-ID":"<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29507,"web_url":"https://patchwork.libcamera.org/comment/29507/","msgid":"","date":"2024-05-09T15:28:14","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nLe jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > From: Nicolas Dufresne \n> > \n> > Test that everything works fine if a buffer outlives the pipeline.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > test/gstreamer/meson.build | 4 +-\n> > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > \n> > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > new file mode 100644\n> > index 00000000..6d932e9e\n> > --- /dev/null\n> > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > @@ -0,0 +1,75 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Nicolas Dufresne\n> > + *\n> > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"gstreamer_test.h\"\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +\n> > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > +{\n> > +public:\n> > + GstreamerMemoryLifetimeTest()\n> > + : GstreamerTest()\n> > + {\n> > + }\n> > +\n> > +protected:\n> > + int init() override\n> > + {\n> > + if (status_ != TestPass)\n> > + return status_;\n> > +\n> > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > + if (!appsink_) {\n> > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > + return TestFail;\n> > + }\n> > + g_object_ref_sink(appsink_);\n> > +\n> > + return createPipeline();\n> > + }\n> > +\n> > + int run() override\n> > + {\n> > + /* Build the pipeline */\n> > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > + g_printerr(\"Elements could not be linked.\\n\");\n> > + return TestFail;\n> > + }\n> > +\n> > + if (startPipeline() != TestPass)\n> > + return TestFail;\n\nDid not add a comment here, since \"startPipeline()\" should be obvious that we\nstart the pipeline. It will raise the state to PLAYING state.\n\n> > +\n> > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n\nThis is also a bit trivial, it pulls a sample from the pipeline.\n\n> > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n\nHere though, I should probably add a comment (well before the change to NULL\nstate).\n\n/* \n * Stop the pipeline without releasing sample_. This ensures that we can hold\n * on memory while elements can release all their internal resources as required\n * by the NULL state definition.\n */\n\n> > +\n> > + if (!sample_)\n> > + return TestFail;\n> \n> Is this the part that does the test? I don't really know how this test\n> is working.\n\nThis is memory safety. As the doc says:\n\nhttps://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n\n> Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n\nAs it annoted nullable, not doing a null check if not safe. I'd, say, it would\nbe annormal, so failing the test make sense, but its not really the expected\nfailure spot for when the code was broken.\n\n> \n> Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> the pipeline?\n\nNo, when moving to STATE_NULL, its is required \n\n> \n> Any comments we can add here to make it clearer what is actually being\n> tested or happening? The code doesn't really tell me what's going on\n> under the hood.\n\nhttps://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n\n> GST_STATE_NULL: this is the default state. No resources are allocated in this\nstate, so, transitioning to it will free all resources. The element must be in \nthis state when its refcount reaches 0 and it is freed.\n\nLet me know if the suggested comment above would do for you.\n\n> \n> Does the test for !sample_ really mean the buffer still exists and is\n> valid? Does the test need to do anything to /access/ the buffer to make\n> sure the test proves it succeeded?\n> \n> I expect it probably does as otherwise you'd have added more - but it's\n> just not obvious from the above.\n> \n> \n\nThe bug being that libcamera crash when the memory is released, in this specific\ncase it crash in cleanup(). But I suspect that we can drop the sample_ member,\nand simple manually unref the sample here (avoiding smart pointer, as it would\nbe equally confusing).\n\np.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\ndon't remember much about it, I'm just reading the patch to reply here.\n\nNicolas\n\n> \n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + void cleanup() override\n> > + {\n> > + g_clear_pointer(&sample_, gst_sample_unref);\n> > + g_clear_object(&appsink_);\n> > + }\n> > +\n> > +private:\n> > + GstElement *appsink_;\n> > + GstSample *sample_;\n> > +};\n> > +\n> > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > index f3ba5a23..70609b88 100644\n> > --- a/test/gstreamer/meson.build\n> > +++ b/test/gstreamer/meson.build\n> > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > ]\n> > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > \n> > foreach test : gstreamer_tests\n> > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > - dependencies : [libcamera_private, gstreamer_dep],\n> > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > link_with : test_libraries,\n> > include_directories : test_includes_internal)\n> > \n> > -- \n> > 2.43.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3C777BDE6B\n\tfor ;\n\tThu, 9 May 2024 15:28:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 198736345F;\n\tThu, 9 May 2024 17:28:22 +0200 (CEST)","from madrid.collaboradmins.com (madrid.collaboradmins.com\n\t[IPv6:2a00:1098:ed:100::25])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44EDC633FA\n\tfor ;\n\tThu, 9 May 2024 17:28:21 +0200 (CEST)","from nicolas-tpx395.localdomain (cola.collaboradmins.com\n\t[195.201.22.229])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madrid.collaboradmins.com (Postfix) with ESMTPSA id 7041F37820FA; \n\tThu, 9 May 2024 15:28:20 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=collabora.com header.i=@collabora.com\n\theader.b=\"5ALiJIkm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1715268500;\n\tbh=laQNEplgvGRauGsgUw+KYZKGCz2Z071Xtg8+9Mvv2js=;\n\th=Subject:From:To:Date:In-Reply-To:References:From;\n\tb=5ALiJIkmot40kaGORPzYmh6A6N9VBtqXQ2IUJPGShmkHiQLnYGFKk+tyjBQL8g+Qn\n\tOeI5u3+aaN9f4gu5sSmZumKyMiJkOkI/+OvqIST6kjxVECMoHLEY8sdxq3ePkaaeub\n\tjBFCJ7QgiON7kjz2fMQwzi89hT8gOi24bKR1OyLUxg9VndyTnSqrALnFtR8IA5lFWf\n\tnJr+CFgvQe0YqKeuPElnrjLEPwc3yhyH/eca5EOIuj63E0VSaRMrbmw7yZnMHgff1Q\n\t1NsUHu3uko+T8WdWGsOcnAHj3UPXIT7pXMGV+KiJLm2OUUUOyU9uSAzf2duNxc7TPJ\n\tDW3LHsfobjW2w==","Message-ID":"","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","From":"Nicolas Dufresne ","To":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 May 2024 11:28:14 -0400","In-Reply-To":"<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.1 (3.52.1-1.fc40) ","MIME-Version":"1.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29511,"web_url":"https://patchwork.libcamera.org/comment/29511/","msgid":"<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>","date":"2024-05-09T16:07:51","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> Hi,\n> \n> Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > From: Nicolas Dufresne \n> > > \n> > > Test that everything works fine if a buffer outlives the pipeline.\n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > test/gstreamer/meson.build | 4 +-\n> > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > \n> > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > new file mode 100644\n> > > index 00000000..6d932e9e\n> > > --- /dev/null\n> > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > @@ -0,0 +1,75 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > + *\n> > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"gstreamer_test.h\"\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +\n> > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > +{\n> > > +public:\n> > > + GstreamerMemoryLifetimeTest()\n> > > + : GstreamerTest()\n> > > + {\n> > > + }\n> > > +\n> > > +protected:\n> > > + int init() override\n> > > + {\n> > > + if (status_ != TestPass)\n> > > + return status_;\n> > > +\n> > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > + if (!appsink_) {\n> > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > + return TestFail;\n> > > + }\n> > > + g_object_ref_sink(appsink_);\n> > > +\n> > > + return createPipeline();\n> > > + }\n> > > +\n> > > + int run() override\n> > > + {\n> > > + /* Build the pipeline */\n> > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (startPipeline() != TestPass)\n> > > + return TestFail;\n> \n> Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> start the pipeline. It will raise the state to PLAYING state.\n\nYes, startPipeline is obvious enough ;-)\n\n\n> > > +\n> > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> \n> This is also a bit trivial, it pulls a sample from the pipeline.\n\nAgreed, but what's the impact in lifetime. do we expect a single buffer?\nI think you're covering it on the comment below though.\n\n> \n> > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> \n> Here though, I should probably add a comment (well before the change to NULL\n> state).\n> \n> /* \n> * Stop the pipeline without releasing sample_. This ensures that we can hold\n> * on memory while elements can release all their internal resources as required\n> * by the NULL state definition.\n> */\n\nYes, that's what I think the test needs to document.\n\n> \n> > > +\n> > > + if (!sample_)\n> > > + return TestFail;\n> > \n> > Is this the part that does the test? I don't really know how this test\n> > is working.\n> \n> This is memory safety. As the doc says:\n> \n> https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n>\n> > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> \n> As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> be annormal, so failing the test make sense, but its not really the expected\n> failure spot for when the code was broken.\n\nWhich is the point that spots the code is broken then? I seem to still\nmiss it ?\n\n\n> \n> > \n> > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > the pipeline?\n> \n> No, when moving to STATE_NULL, its is required \n> \n> > \n> > Any comments we can add here to make it clearer what is actually being\n> > tested or happening? The code doesn't really tell me what's going on\n> > under the hood.\n> \n> https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> \n> > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> state, so, transitioning to it will free all resources. The element must be in \n> this state when its refcount reaches 0 and it is freed.\n> \n> Let me know if the suggested comment above would do for you.\n> \n> > \n> > Does the test for !sample_ really mean the buffer still exists and is\n> > valid? Does the test need to do anything to /access/ the buffer to make\n> > sure the test proves it succeeded?\n> > \n> > I expect it probably does as otherwise you'd have added more - but it's\n> > just not obvious from the above.\n> > \n> > \n> \n> The bug being that libcamera crash when the memory is released, in this specific\n> case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> and simple manually unref the sample here (avoiding smart pointer, as it would\n> be equally confusing).\n> \n> p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> don't remember much about it, I'm just reading the patch to reply here.\n\nSounds like a great reason to make sure we have comments in the code\nthat explain the 'why' :-)\n\nI'm fine with whatever comments you believe are appropriate. 'You' two\nmonths later are probably the best reviewer here ;-)\n\n\n\n> \n> Nicolas\n> \n> > \n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + void cleanup() override\n> > > + {\n\nAre you saying the test is actually testing the issue /here/ ? If so -\ncan we add a comment explicitly saying that ?\n\nOtherwise this is just 'cleanup' code to me. That's why I was looking at\nthe null check of sample_ trying to figure out if that's how you were\ndetermining if the test had failed (Becuase that's the only time you\nreturn TestFail after the test actually starts).\n\n\nI guess the failure point is that the test will crash, rather than\ndetect a failure. Would that also be noteworthy?\n\n\n> > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > + g_clear_object(&appsink_);\n> > > + }\n> > > +\n> > > +private:\n> > > + GstElement *appsink_;\n> > > + GstSample *sample_;\n> > > +};\n> > > +\n> > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > index f3ba5a23..70609b88 100644\n> > > --- a/test/gstreamer/meson.build\n> > > +++ b/test/gstreamer/meson.build\n> > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > ]\n> > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > \n> > > foreach test : gstreamer_tests\n> > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > link_with : test_libraries,\n> > > include_directories : test_includes_internal)\n> > > \n> > > -- \n> > > 2.43.2\n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24AC6C3226\n\tfor ;\n\tThu, 9 May 2024 16:07:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3ACC6345F;\n\tThu, 9 May 2024 18:07:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 171FE633FA\n\tfor ;\n\tThu, 9 May 2024 18:07:54 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D67D904;\n\tThu, 9 May 2024 18:07:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OuTUS90W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715270870;\n\tbh=TpGZE5/7ZhpNBWkSWCnF/amuRpsKihtxEQ/pvEH8u1M=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OuTUS90W6tuPqkexWqLP7rHqqbwsRGItVxU7L2xpOlIEYlUK3ug8R8qxDC6zqqmsh\n\trshJkyetGAFJF8LbAYleyTqBBPcR/70V97UKnj1Z8Nnc21fwm560ZlhCI7w0jN11Ye\n\t6lrPSpyoXmtH1oHlx1dCt0LookBtn30R9T7GFgXI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","From":"Kieran Bingham ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 May 2024 17:07:51 +0100","Message-ID":"<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29516,"web_url":"https://patchwork.libcamera.org/comment/29516/","msgid":"<20240512223504.GU17158@pendragon.ideasonboard.com>","date":"2024-05-12T22:35:04","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:\n> Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> > Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > > From: Nicolas Dufresne \n> > > > \n> > > > Test that everything works fine if a buffer outlives the pipeline.\n> > > > \n> > > > Signed-off-by: Nicolas Dufresne \n> > > > ---\n> > > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > > test/gstreamer/meson.build | 4 +-\n> > > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > \n> > > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > new file mode 100644\n> > > > index 00000000..6d932e9e\n> > > > --- /dev/null\n> > > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > @@ -0,0 +1,75 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > > + *\n> > > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"gstreamer_test.h\"\n> > > > +#include \"test.h\"\n> > > > +\n> > > > +using namespace std;\n> > > > +\n> > > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > > +{\n> > > > +public:\n> > > > + GstreamerMemoryLifetimeTest()\n> > > > + : GstreamerTest()\n> > > > + {\n> > > > + }\n> > > > +\n> > > > +protected:\n> > > > + int init() override\n> > > > + {\n> > > > + if (status_ != TestPass)\n> > > > + return status_;\n> > > > +\n> > > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > > + if (!appsink_) {\n> > > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > + g_object_ref_sink(appsink_);\n> > > > +\n> > > > + return createPipeline();\n> > > > + }\n> > > > +\n> > > > + int run() override\n> > > > + {\n> > > > + /* Build the pipeline */\n> > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + if (startPipeline() != TestPass)\n> > > > + return TestFail;\n> > \n> > Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> > start the pipeline. It will raise the state to PLAYING state.\n> \n> Yes, startPipeline is obvious enough ;-)\n> \n> > > > +\n> > > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> > \n> > This is also a bit trivial, it pulls a sample from the pipeline.\n> \n> Agreed, but what's the impact in lifetime. do we expect a single buffer?\n> I think you're covering it on the comment below though.\n> \n> > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > \n> > Here though, I should probably add a comment (well before the change to NULL\n> > state).\n> > \n> > /* \n> > * Stop the pipeline without releasing sample_. This ensures that we can hold\n> > * on memory while elements can release all their internal resources as required\n> > * by the NULL state definition.\n> > */\n> \n> Yes, that's what I think the test needs to document.\n\n+1. That's also the part I wasn't sure about.\n\n> > > > +\n> > > > + if (!sample_)\n> > > > + return TestFail;\n> > > \n> > > Is this the part that does the test? I don't really know how this test\n> > > is working.\n> > \n> > This is memory safety. As the doc says:\n> > \n> > https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n> >\n> > > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> > \n> > As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> > be annormal, so failing the test make sense, but its not really the expected\n> > failure spot for when the code was broken.\n\nShould/can it be checked before calling gst_element_set_state(), or is\nthe order above important ? sample_ being a normal pointer I assume that\nthe call to gst_element_set_state() won't modify it, but the order of\nthe code made it feel like there was some magic happening.\n\n> Which is the point that spots the code is broken then? I seem to still\n> miss it ?\n> \n> > > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > > the pipeline?\n> > \n> > No, when moving to STATE_NULL, its is required \n> > \n> > > Any comments we can add here to make it clearer what is actually being\n> > > tested or happening? The code doesn't really tell me what's going on\n> > > under the hood.\n> > \n> > https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> > \n> > > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> > state, so, transitioning to it will free all resources. The element must be in \n> > this state when its refcount reaches 0 and it is freed.\n> > \n> > Let me know if the suggested comment above would do for you.\n> > \n> > > Does the test for !sample_ really mean the buffer still exists and is\n> > > valid? Does the test need to do anything to /access/ the buffer to make\n> > > sure the test proves it succeeded?\n> > > \n> > > I expect it probably does as otherwise you'd have added more - but it's\n> > > just not obvious from the above.\n> > \n> > The bug being that libcamera crash when the memory is released, in this specific\n> > case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> > and simple manually unref the sample here (avoiding smart pointer, as it would\n> > be equally confusing).\n> > \n> > p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> > don't remember much about it, I'm just reading the patch to reply here.\n\nSorry for not replying earlier.\n\n> Sounds like a great reason to make sure we have comments in the code\n> that explain the 'why' :-)\n> \n> I'm fine with whatever comments you believe are appropriate. 'You' two\n> months later are probably the best reviewer here ;-)\n\nAnother comment about this patch in particular. We tend to order this\nkind of series with the test coming first, and the fix coming next. This\nallows ensuring that the test fails, and that the fix fixes the issue.\nThis guards against cases where the test would be incorrect and wouldn't\nshowcase the actual problem, and the fix would also be bogus.\n\n> > > > +\n> > > > + return TestPass;\n> > > > + }\n> > > > +\n> > > > + void cleanup() override\n> > > > + {\n> \n> Are you saying the test is actually testing the issue /here/ ? If so -\n> can we add a comment explicitly saying that ?\n> \n> Otherwise this is just 'cleanup' code to me. That's why I was looking at\n> the null check of sample_ trying to figure out if that's how you were\n> determining if the test had failed (Becuase that's the only time you\n> return TestFail after the test actually starts).\n> \n> \n> I guess the failure point is that the test will crash, rather than\n> detect a failure. Would that also be noteworthy?\n\nThat is worth a comment, yes.\n\n> > > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > > + g_clear_object(&appsink_);\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + GstElement *appsink_;\n> > > > + GstSample *sample_;\n> > > > +};\n> > > > +\n> > > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > index f3ba5a23..70609b88 100644\n> > > > --- a/test/gstreamer/meson.build\n> > > > +++ b/test/gstreamer/meson.build\n> > > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > > ]\n> > > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > > \n> > > > foreach test : gstreamer_tests\n> > > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > > link_with : test_libraries,\n> > > > include_directories : test_includes_internal)\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B9A4CC3226\n\tfor ;\n\tSun, 12 May 2024 22:35:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77F4B6347C;\n\tMon, 13 May 2024 00:35:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1D9861A66\n\tfor ;\n\tMon, 13 May 2024 00:35:12 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C11AE397;\n\tMon, 13 May 2024 00:35:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"K4OLrWZz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715553307;\n\tbh=A1GhRtn02GtHikTN7aZ3psQZHb+RY595uB/O114gVVY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K4OLrWZzh2E1+daTkE+nxKngoDmd2SCl5DkTIHCauF970zBU1xXG9OPFlmLrlUTIK\n\tVABVqPV4kPOLmszrqmchzFmwOrwbZF+ZbzEY2p+D/7CNgtWmN7UFvpCBP8Bk8JMh/G\n\tqR4NbPHc8xbkLjLca6quGb7RdRn77kUCGIj3pzwA=","Date":"Mon, 13 May 2024 01:35:04 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","Message-ID":"<20240512223504.GU17158@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t\n\t<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29522,"web_url":"https://patchwork.libcamera.org/comment/29522/","msgid":"<20240513133218.GC18630@pendragon.ideasonboard.com>","date":"2024-05-13T13:32:18","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, May 13, 2024 at 01:35:04AM +0300, Laurent Pinchart wrote:\n> On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:\n> > Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> > > Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > > > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > > > From: Nicolas Dufresne \n> > > > > \n> > > > > Test that everything works fine if a buffer outlives the pipeline.\n> > > > > \n> > > > > Signed-off-by: Nicolas Dufresne \n> > > > > ---\n> > > > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > > > test/gstreamer/meson.build | 4 +-\n> > > > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > \n> > > > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..6d932e9e\n> > > > > --- /dev/null\n> > > > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > @@ -0,0 +1,75 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > > > + *\n> > > > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"gstreamer_test.h\"\n> > > > > +#include \"test.h\"\n> > > > > +\n> > > > > +using namespace std;\n> > > > > +\n> > > > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > > > +{\n> > > > > +public:\n> > > > > + GstreamerMemoryLifetimeTest()\n> > > > > + : GstreamerTest()\n> > > > > + {\n> > > > > + }\n> > > > > +\n> > > > > +protected:\n> > > > > + int init() override\n> > > > > + {\n> > > > > + if (status_ != TestPass)\n> > > > > + return status_;\n> > > > > +\n> > > > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > > > + if (!appsink_) {\n> > > > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > + g_object_ref_sink(appsink_);\n> > > > > +\n> > > > > + return createPipeline();\n> > > > > + }\n> > > > > +\n> > > > > + int run() override\n> > > > > + {\n> > > > > + /* Build the pipeline */\n> > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + if (startPipeline() != TestPass)\n> > > > > + return TestFail;\n> > > \n> > > Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> > > start the pipeline. It will raise the state to PLAYING state.\n> > \n> > Yes, startPipeline is obvious enough ;-)\n> > \n> > > > > +\n> > > > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> > > \n> > > This is also a bit trivial, it pulls a sample from the pipeline.\n> > \n> > Agreed, but what's the impact in lifetime. do we expect a single buffer?\n> > I think you're covering it on the comment below though.\n> > \n> > > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > \n> > > Here though, I should probably add a comment (well before the change to NULL\n> > > state).\n> > > \n> > > /* \n> > > * Stop the pipeline without releasing sample_. This ensures that we can hold\n> > > * on memory while elements can release all their internal resources as required\n> > > * by the NULL state definition.\n> > > */\n> > \n> > Yes, that's what I think the test needs to document.\n> \n> +1. That's also the part I wasn't sure about.\n> \n> > > > > +\n> > > > > + if (!sample_)\n> > > > > + return TestFail;\n> > > > \n> > > > Is this the part that does the test? I don't really know how this test\n> > > > is working.\n> > > \n> > > This is memory safety. As the doc says:\n> > > \n> > > https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n> > >\n> > > > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> > > \n> > > As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> > > be annormal, so failing the test make sense, but its not really the expected\n> > > failure spot for when the code was broken.\n> \n> Should/can it be checked before calling gst_element_set_state(), or is\n> the order above important ? sample_ being a normal pointer I assume that\n> the call to gst_element_set_state() won't modify it, but the order of\n> the code made it feel like there was some magic happening.\n> \n> > Which is the point that spots the code is broken then? I seem to still\n> > miss it ?\n> > \n> > > > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > > > the pipeline?\n> > > \n> > > No, when moving to STATE_NULL, its is required \n> > > \n> > > > Any comments we can add here to make it clearer what is actually being\n> > > > tested or happening? The code doesn't really tell me what's going on\n> > > > under the hood.\n> > > \n> > > https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> > > \n> > > > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> > > state, so, transitioning to it will free all resources. The element must be in \n> > > this state when its refcount reaches 0 and it is freed.\n> > > \n> > > Let me know if the suggested comment above would do for you.\n> > > \n> > > > Does the test for !sample_ really mean the buffer still exists and is\n> > > > valid? Does the test need to do anything to /access/ the buffer to make\n> > > > sure the test proves it succeeded?\n> > > > \n> > > > I expect it probably does as otherwise you'd have added more - but it's\n> > > > just not obvious from the above.\n> > > \n> > > The bug being that libcamera crash when the memory is released, in this specific\n> > > case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> > > and simple manually unref the sample here (avoiding smart pointer, as it would\n> > > be equally confusing).\n> > > \n> > > p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> > > don't remember much about it, I'm just reading the patch to reply here.\n> \n> Sorry for not replying earlier.\n> \n> > Sounds like a great reason to make sure we have comments in the code\n> > that explain the 'why' :-)\n> > \n> > I'm fine with whatever comments you believe are appropriate. 'You' two\n> > months later are probably the best reviewer here ;-)\n> \n> Another comment about this patch in particular. We tend to order this\n> kind of series with the test coming first, and the fix coming next. This\n> allows ensuring that the test fails, and that the fix fixes the issue.\n> This guards against cases where the test would be incorrect and wouldn't\n> showcase the actual problem, and the fix would also be bogus.\n\nI confirm that this patch, without patch 1/3, causes a crash when\nlibcamera is compiled without ASan, and an ASan failure otherwise.\n\nTested-by: Laurent Pinchart \n\n> > > > > +\n> > > > > + return TestPass;\n> > > > > + }\n> > > > > +\n> > > > > + void cleanup() override\n> > > > > + {\n> > \n> > Are you saying the test is actually testing the issue /here/ ? If so -\n> > can we add a comment explicitly saying that ?\n> > \n> > Otherwise this is just 'cleanup' code to me. That's why I was looking at\n> > the null check of sample_ trying to figure out if that's how you were\n> > determining if the test had failed (Becuase that's the only time you\n> > return TestFail after the test actually starts).\n> > \n> > \n> > I guess the failure point is that the test will crash, rather than\n> > detect a failure. Would that also be noteworthy?\n> \n> That is worth a comment, yes.\n> \n> > > > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > > > + g_clear_object(&appsink_);\n> > > > > + }\n> > > > > +\n> > > > > +private:\n> > > > > + GstElement *appsink_;\n> > > > > + GstSample *sample_;\n> > > > > +};\n> > > > > +\n> > > > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > index f3ba5a23..70609b88 100644\n> > > > > --- a/test/gstreamer/meson.build\n> > > > > +++ b/test/gstreamer/meson.build\n> > > > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > > > ]\n> > > > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > > > \n> > > > > foreach test : gstreamer_tests\n> > > > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > > > link_with : test_libraries,\n> > > > > include_directories : test_includes_internal)\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A16A4C3226\n\tfor ;\n\tMon, 13 May 2024 13:32:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97DF26346B;\n\tMon, 13 May 2024 15:32:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B643A61A5E\n\tfor ;\n\tMon, 13 May 2024 15:32:26 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 11D50397;\n\tMon, 13 May 2024 15:32:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GWlUN3aH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715607140;\n\tbh=oo3cpkX902oII196zAUFuILajeFzAlvDj9oc81it94g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GWlUN3aHmuoOidDai4HrHpG+3qT/6kBUGRNcaPdSIA7n+ZTbjI5A9yqSJvPqp4Ewa\n\tfyPufp2wtUIv/KioQ/pPk+IFUFmG4D5F67Jzhz7HM9HBPgDe/01EjqCnBfE/w1EqOt\n\t/kVv+YRV3B/JOj10HQgukxkc8knEuESe11faPadU=","Date":"Mon, 13 May 2024 16:32:18 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","Message-ID":"<20240513133218.GC18630@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t\n\t<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>\n\t<20240512223504.GU17158@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20240512223504.GU17158@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29524,"web_url":"https://patchwork.libcamera.org/comment/29524/","msgid":"<020c1831147138dd84e81e5b8168d14045024e24.camel@collabora.com>","date":"2024-05-13T15:18:01","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nLe lundi 13 mai 2024 à 01:35 +0300, Laurent Pinchart a écrit :\n> On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:\n> > Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> > > Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > > > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > > > From: Nicolas Dufresne \n> > > > > \n> > > > > Test that everything works fine if a buffer outlives the pipeline.\n> > > > > \n> > > > > Signed-off-by: Nicolas Dufresne \n> > > > > ---\n> > > > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > > > test/gstreamer/meson.build | 4 +-\n> > > > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > \n> > > > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..6d932e9e\n> > > > > --- /dev/null\n> > > > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > @@ -0,0 +1,75 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > > > + *\n> > > > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"gstreamer_test.h\"\n> > > > > +#include \"test.h\"\n> > > > > +\n> > > > > +using namespace std;\n> > > > > +\n> > > > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > > > +{\n> > > > > +public:\n> > > > > + GstreamerMemoryLifetimeTest()\n> > > > > + : GstreamerTest()\n> > > > > + {\n> > > > > + }\n> > > > > +\n> > > > > +protected:\n> > > > > + int init() override\n> > > > > + {\n> > > > > + if (status_ != TestPass)\n> > > > > + return status_;\n> > > > > +\n> > > > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > > > + if (!appsink_) {\n> > > > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > + g_object_ref_sink(appsink_);\n> > > > > +\n> > > > > + return createPipeline();\n> > > > > + }\n> > > > > +\n> > > > > + int run() override\n> > > > > + {\n> > > > > + /* Build the pipeline */\n> > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > > + return TestFail;\n> > > > > + }\n> > > > > +\n> > > > > + if (startPipeline() != TestPass)\n> > > > > + return TestFail;\n> > > \n> > > Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> > > start the pipeline. It will raise the state to PLAYING state.\n> > \n> > Yes, startPipeline is obvious enough ;-)\n> > \n> > > > > +\n> > > > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> > > \n> > > This is also a bit trivial, it pulls a sample from the pipeline.\n> > \n> > Agreed, but what's the impact in lifetime. do we expect a single buffer?\n> > I think you're covering it on the comment below though.\n> > \n> > > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > \n> > > Here though, I should probably add a comment (well before the change to NULL\n> > > state).\n> > > \n> > > /* \n> > > * Stop the pipeline without releasing sample_. This ensures that we can hold\n> > > * on memory while elements can release all their internal resources as required\n> > > * by the NULL state definition.\n> > > */\n> > \n> > Yes, that's what I think the test needs to document.\n> \n> +1. That's also the part I wasn't sure about.\n> \n> > > > > +\n> > > > > + if (!sample_)\n> > > > > + return TestFail;\n> > > > \n> > > > Is this the part that does the test? I don't really know how this test\n> > > > is working.\n> > > \n> > > This is memory safety. As the doc says:\n> > > \n> > > https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n> > > \n> > > > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> > > \n> > > As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> > > be annormal, so failing the test make sense, but its not really the expected\n> > > failure spot for when the code was broken.\n> \n> Should/can it be checked before calling gst_element_set_state(), or is\n> the order above important ? sample_ being a normal pointer I assume that\n> the call to gst_element_set_state() won't modify it, but the order of\n> the code made it feel like there was some magic happening.\n\nIf you prefer, we can duplicate the set_state call, here's the pseudo code of\nwhat can be done, also added a draft of the missing comment.\n\n sample = pull() \n if (!sample) { \n set_state(NULL); return failed; \n } set_state(NULL);\n\n /* \n * Keep the sample referenced. This way, it will be released \n * in cleanup() after the camera manager object. This used to \n * lead to a crash as freeing video frames tried to call into \n * the freed camera manager. \n */ \n return success;\n\nThe reason being that behaviour is undefined (and document to deadlock most of\nthe time) if you drop the last reference of a pipeline that is running (any\nstate above NULL).\n\n> \n> > Which is the point that spots the code is broken then? I seem to still\n> > miss it ?\n> > \n> > > > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > > > the pipeline?\n> > > \n> > > No, when moving to STATE_NULL, its is required \n> > > \n> > > > Any comments we can add here to make it clearer what is actually being\n> > > > tested or happening? The code doesn't really tell me what's going on\n> > > > under the hood.\n> > > \n> > > https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> > > \n> > > > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> > > state, so, transitioning to it will free all resources. The element must be in \n> > > this state when its refcount reaches 0 and it is freed.\n> > > \n> > > Let me know if the suggested comment above would do for you.\n> > > \n> > > > Does the test for !sample_ really mean the buffer still exists and is\n> > > > valid? Does the test need to do anything to /access/ the buffer to make\n> > > > sure the test proves it succeeded?\n> > > > \n> > > > I expect it probably does as otherwise you'd have added more - but it's\n> > > > just not obvious from the above.\n> > > \n> > > The bug being that libcamera crash when the memory is released, in this specific\n> > > case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> > > and simple manually unref the sample here (avoiding smart pointer, as it would\n> > > be equally confusing).\n> > > \n> > > p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> > > don't remember much about it, I'm just reading the patch to reply here.\n> \n> Sorry for not replying earlier.\n> \n> > Sounds like a great reason to make sure we have comments in the code\n> > that explain the 'why' :-)\n> > \n> > I'm fine with whatever comments you believe are appropriate. 'You' two\n> > months later are probably the best reviewer here ;-)\n> \n> Another comment about this patch in particular. We tend to order this\n> kind of series with the test coming first, and the fix coming next. This\n> allows ensuring that the test fails, and that the fix fixes the issue.\n> This guards against cases where the test would be incorrect and wouldn't\n> showcase the actual problem, and the fix would also be bogus.\n\nSure, as we discussed on IRC, in my other projects we do the total opposite to\navoid making bisection painful. Might be something to revisit later for you.\n\nNicolas\n\n> \n> > > > > +\n> > > > > + return TestPass;\n> > > > > + }\n> > > > > +\n> > > > > + void cleanup() override\n> > > > > + {\n> > \n> > Are you saying the test is actually testing the issue /here/ ? If so -\n> > can we add a comment explicitly saying that ?\n> > \n> > Otherwise this is just 'cleanup' code to me. That's why I was looking at\n> > the null check of sample_ trying to figure out if that's how you were\n> > determining if the test had failed (Becuase that's the only time you\n> > return TestFail after the test actually starts).\n> > \n> > \n> > I guess the failure point is that the test will crash, rather than\n> > detect a failure. Would that also be noteworthy?\n> \n> That is worth a comment, yes.\n> \n> > > > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > > > + g_clear_object(&appsink_);\n> > > > > + }\n> > > > > +\n> > > > > +private:\n> > > > > + GstElement *appsink_;\n> > > > > + GstSample *sample_;\n> > > > > +};\n> > > > > +\n> > > > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > index f3ba5a23..70609b88 100644\n> > > > > --- a/test/gstreamer/meson.build\n> > > > > +++ b/test/gstreamer/meson.build\n> > > > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > > > ]\n> > > > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > > > \n> > > > > foreach test : gstreamer_tests\n> > > > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > > > link_with : test_libraries,\n> > > > > include_directories : test_includes_internal)\n> > > > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CDD94C3226\n\tfor ;\n\tMon, 13 May 2024 15:18:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7E336346B;\n\tMon, 13 May 2024 17:18:09 +0200 (CEST)","from madrid.collaboradmins.com (madrid.collaboradmins.com\n\t[46.235.227.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBA7A61A5E\n\tfor ;\n\tMon, 13 May 2024 17:18:08 +0200 (CEST)","from nicolas-tpx395.localdomain (cola.collaboradmins.com\n\t[195.201.22.229])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madrid.collaboradmins.com (Postfix) with ESMTPSA id D5A6D37810F1; \n\tMon, 13 May 2024 15:18:07 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=collabora.com header.i=@collabora.com\n\theader.b=\"FP1LCQs/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1715613488;\n\tbh=gHFrn/XjbG9X54rnaF6HQUpAaLt/z5976fuXyVZ9eyQ=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=FP1LCQs/2+M/OIQnf0z4jRwjTE13LCk5z9nmNXcqdvA3xTolhejQr3ASgkQRrT7Fu\n\tlczoynahTig2Amt8TDFlZgwVlaYcuFRlefep/osD39NxF8evvoS8Xja2Nv7J0d16u2\n\tCm7mQUVt0802ug2p0XsXxD3VxzTtVkcUjLQ8YdBcG4p8UPg7ydMKCohqqrYi6CSUlK\n\tvWOKa22DSxRs34lh/jtU7SgjEeHqS21VMoLHB7i8oakaLTzfOopWdiZa9RJXtQASCo\n\tOhTvQCffvWSIMOGpRocB/DxlpE3UJ0GZAE8y5r0OY58IQEhUnGbtq+FJMj3VaKf4L0\n\tjijDwdJ/Wtc/w==","Message-ID":"<020c1831147138dd84e81e5b8168d14045024e24.camel@collabora.com>","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Kieran Bingham\n\t","Cc":"libcamera-devel@lists.libcamera.org","Date":"Mon, 13 May 2024 11:18:01 -0400","In-Reply-To":"<20240512223504.GU17158@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t\n\t<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>\n\t<20240512223504.GU17158@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.1 (3.52.1-1.fc40) ","MIME-Version":"1.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29526,"web_url":"https://patchwork.libcamera.org/comment/29526/","msgid":"<20240514160216.GH32013@pendragon.ideasonboard.com>","date":"2024-05-14T16:02:16","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, May 13, 2024 at 11:18:01AM -0400, Nicolas Dufresne wrote:\n> Le lundi 13 mai 2024 à 01:35 +0300, Laurent Pinchart a écrit :\n> > On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:\n> > > Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> > > > Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > > > > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > > > > From: Nicolas Dufresne \n> > > > > > \n> > > > > > Test that everything works fine if a buffer outlives the pipeline.\n> > > > > > \n> > > > > > Signed-off-by: Nicolas Dufresne \n> > > > > > ---\n> > > > > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > > > > test/gstreamer/meson.build | 4 +-\n> > > > > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > > > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > \n> > > > > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..6d932e9e\n> > > > > > --- /dev/null\n> > > > > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > @@ -0,0 +1,75 @@\n> > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > > > > + *\n> > > > > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"gstreamer_test.h\"\n> > > > > > +#include \"test.h\"\n> > > > > > +\n> > > > > > +using namespace std;\n> > > > > > +\n> > > > > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + GstreamerMemoryLifetimeTest()\n> > > > > > + : GstreamerTest()\n> > > > > > + {\n> > > > > > + }\n> > > > > > +\n> > > > > > +protected:\n> > > > > > + int init() override\n> > > > > > + {\n> > > > > > + if (status_ != TestPass)\n> > > > > > + return status_;\n> > > > > > +\n> > > > > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > > > > + if (!appsink_) {\n> > > > > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > + g_object_ref_sink(appsink_);\n> > > > > > +\n> > > > > > + return createPipeline();\n> > > > > > + }\n> > > > > > +\n> > > > > > + int run() override\n> > > > > > + {\n> > > > > > + /* Build the pipeline */\n> > > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > > > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > > > + return TestFail;\n> > > > > > + }\n> > > > > > +\n> > > > > > + if (startPipeline() != TestPass)\n> > > > > > + return TestFail;\n> > > > \n> > > > Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> > > > start the pipeline. It will raise the state to PLAYING state.\n> > > \n> > > Yes, startPipeline is obvious enough ;-)\n> > > \n> > > > > > +\n> > > > > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> > > > \n> > > > This is also a bit trivial, it pulls a sample from the pipeline.\n> > > \n> > > Agreed, but what's the impact in lifetime. do we expect a single buffer?\n> > > I think you're covering it on the comment below though.\n> > > \n> > > > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > > \n> > > > Here though, I should probably add a comment (well before the change to NULL\n> > > > state).\n> > > > \n> > > > /* \n> > > > * Stop the pipeline without releasing sample_. This ensures that we can hold\n> > > > * on memory while elements can release all their internal resources as required\n> > > > * by the NULL state definition.\n> > > > */\n> > > \n> > > Yes, that's what I think the test needs to document.\n> > \n> > +1. That's also the part I wasn't sure about.\n> > \n> > > > > > +\n> > > > > > + if (!sample_)\n> > > > > > + return TestFail;\n> > > > > \n> > > > > Is this the part that does the test? I don't really know how this test\n> > > > > is working.\n> > > > \n> > > > This is memory safety. As the doc says:\n> > > > \n> > > > https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n> > > > \n> > > > > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> > > > \n> > > > As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> > > > be annormal, so failing the test make sense, but its not really the expected\n> > > > failure spot for when the code was broken.\n> > \n> > Should/can it be checked before calling gst_element_set_state(), or is\n> > the order above important ? sample_ being a normal pointer I assume that\n> > the call to gst_element_set_state() won't modify it, but the order of\n> > the code made it feel like there was some magic happening.\n> \n> If you prefer, we can duplicate the set_state call, here's the pseudo code of\n> what can be done, also added a draft of the missing comment.\n> \n> sample = pull() \n> if (!sample) { \n> set_state(NULL); return failed; \n> } set_state(NULL);\n> \n> /* \n> * Keep the sample referenced. This way, it will be released \n> * in cleanup() after the camera manager object. This used to \n> * lead to a crash as freeing video frames tried to call into \n> * the freed camera manager. \n> */ \n> return success;\n\nNormally I would go for the code included in this patch, it's more\nefficient. In this specific case, as the code is a test case, I think\nit's important to make it as clear as possible. How about the following\n(which verifies my good understanding of the test case) ?\n\n\tsample = pull() \n\tif (!sample) { \n\t\tset_state(NULL);\n\t\treturn failed;\n\t}\n\n\t/* \n\t * Keep the sample referenced and set the pipeline state to NULL. This\n\t * causes the libcamerasrc element to synchronously release resources it\n\t * holds. The sample will be released later in cleanup().\n\t *\n\t * The test case verifies that libcamerasrc keeps alive long enough all\n\t * the resources that are needed until memory allocated for frames gets\n\t * freed. Any use-after-free will be caught by the test crashing in\n\t * cleanup().\n\t */ \n\tset_state(NULL);\n\t return success;\n\n> The reason being that behaviour is undefined (and document to deadlock most of\n> the time) if you drop the last reference of a pipeline that is running (any\n> state above NULL).\n> \n> > > Which is the point that spots the code is broken then? I seem to still\n> > > miss it ?\n> > > \n> > > > > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > > > > the pipeline?\n> > > > \n> > > > No, when moving to STATE_NULL, its is required \n> > > > \n> > > > > Any comments we can add here to make it clearer what is actually being\n> > > > > tested or happening? The code doesn't really tell me what's going on\n> > > > > under the hood.\n> > > > \n> > > > https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> > > > \n> > > > > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> > > > state, so, transitioning to it will free all resources. The element must be in \n> > > > this state when its refcount reaches 0 and it is freed.\n> > > > \n> > > > Let me know if the suggested comment above would do for you.\n> > > > \n> > > > > Does the test for !sample_ really mean the buffer still exists and is\n> > > > > valid? Does the test need to do anything to /access/ the buffer to make\n> > > > > sure the test proves it succeeded?\n> > > > > \n> > > > > I expect it probably does as otherwise you'd have added more - but it's\n> > > > > just not obvious from the above.\n> > > > \n> > > > The bug being that libcamera crash when the memory is released, in this specific\n> > > > case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> > > > and simple manually unref the sample here (avoiding smart pointer, as it would\n> > > > be equally confusing).\n> > > > \n> > > > p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> > > > don't remember much about it, I'm just reading the patch to reply here.\n> > \n> > Sorry for not replying earlier.\n> > \n> > > Sounds like a great reason to make sure we have comments in the code\n> > > that explain the 'why' :-)\n> > > \n> > > I'm fine with whatever comments you believe are appropriate. 'You' two\n> > > months later are probably the best reviewer here ;-)\n> > \n> > Another comment about this patch in particular. We tend to order this\n> > kind of series with the test coming first, and the fix coming next. This\n> > allows ensuring that the test fails, and that the fix fixes the issue.\n> > This guards against cases where the test would be incorrect and wouldn't\n> > showcase the actual problem, and the fix would also be bogus.\n> \n> Sure, as we discussed on IRC, in my other projects we do the total opposite to\n> avoid making bisection painful. Might be something to revisit later for you.\n\nSure. We could also mark the test as expected to fail, and turn it into\nexpected to succeed in the patch that fixes the issue.\n\n> > > > > > +\n> > > > > > + return TestPass;\n> > > > > > + }\n> > > > > > +\n> > > > > > + void cleanup() override\n> > > > > > + {\n> > > \n> > > Are you saying the test is actually testing the issue /here/ ? If so -\n> > > can we add a comment explicitly saying that ?\n> > > \n> > > Otherwise this is just 'cleanup' code to me. That's why I was looking at\n> > > the null check of sample_ trying to figure out if that's how you were\n> > > determining if the test had failed (Becuase that's the only time you\n> > > return TestFail after the test actually starts).\n> > > \n> > > \n> > > I guess the failure point is that the test will crash, rather than\n> > > detect a failure. Would that also be noteworthy?\n> > \n> > That is worth a comment, yes.\n> > \n> > > > > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > > > > + g_clear_object(&appsink_);\n> > > > > > + }\n> > > > > > +\n> > > > > > +private:\n> > > > > > + GstElement *appsink_;\n> > > > > > + GstSample *sample_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > > index f3ba5a23..70609b88 100644\n> > > > > > --- a/test/gstreamer/meson.build\n> > > > > > +++ b/test/gstreamer/meson.build\n> > > > > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > > > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > > > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > > > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > > > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > > > > ]\n> > > > > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > > > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > > > > \n> > > > > > foreach test : gstreamer_tests\n> > > > > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > > > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > > > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > > > > link_with : test_libraries,\n> > > > > > include_directories : test_includes_internal)\n> > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7AD47BDE6B\n\tfor ;\n\tTue, 14 May 2024 16:02:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D8F86347E;\n\tTue, 14 May 2024 18:02:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D413263469\n\tfor ;\n\tTue, 14 May 2024 18:02:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53B8627C;\n\tTue, 14 May 2024 18:02:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eiSX5esl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715702537;\n\tbh=ul5Kesb2sln3GMG4p8nPZIn3yjMA84C705zPf9KB2nw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eiSX5eslK5vyTpcH7vB6pekJ2wvC97wvKKLwFwE2/S2UVfOLnNhuN1+rz3jrM9UaR\n\tpOR2C/0egDMzo1/Ukpbd1fg0qfjNrlfYxsNcjCiTctdtokaQWtxdmGMIgqU1ITIthC\n\tvAJ7QU5MUKChQK1oMSesw/7IA6cy1BUl1JRscDbU=","Date":"Tue, 14 May 2024 19:02:16 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","Message-ID":"<20240514160216.GH32013@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t\n\t<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>\n\t<20240512223504.GU17158@pendragon.ideasonboard.com>\n\t<020c1831147138dd84e81e5b8168d14045024e24.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<020c1831147138dd84e81e5b8168d14045024e24.camel@collabora.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30482,"web_url":"https://patchwork.libcamera.org/comment/30482/","msgid":"<172190451318.392292.4588062676990013289@ping.linuxembedded.co.uk>","date":"2024-07-25T10:48:33","subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-05-14 17:02:16)\n> Hi Nicolas,\n> \n> On Mon, May 13, 2024 at 11:18:01AM -0400, Nicolas Dufresne wrote:\n> > Le lundi 13 mai 2024 à 01:35 +0300, Laurent Pinchart a écrit :\n> > > On Thu, May 09, 2024 at 05:07:51PM +0100, Kieran Bingham wrote:\n> > > > Quoting Nicolas Dufresne (2024-05-09 16:28:14)\n> > > > > Le jeudi 09 mai 2024 à 15:57 +0100, Kieran Bingham a écrit :\n> > > > > > Quoting Nicolas Dufresne (2024-03-05 15:30:58)\n> > > > > > > From: Nicolas Dufresne \n> > > > > > > \n> > > > > > > Test that everything works fine if a buffer outlives the pipeline.\n> > > > > > > \n> > > > > > > Signed-off-by: Nicolas Dufresne \n> > > > > > > ---\n> > > > > > > .../gstreamer_memory_lifetime_test.cpp | 75 +++++++++++++++++++\n> > > > > > > test/gstreamer/meson.build | 4 +-\n> > > > > > > 2 files changed, 78 insertions(+), 1 deletion(-)\n> > > > > > > create mode 100644 test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > > \n> > > > > > > diff --git a/test/gstreamer/gstreamer_memory_lifetime_test.cpp b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > > new file mode 100644\n> > > > > > > index 00000000..6d932e9e\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/test/gstreamer/gstreamer_memory_lifetime_test.cpp\n> > > > > > > @@ -0,0 +1,75 @@\n> > > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2024, Nicolas Dufresne\n> > > > > > > + *\n> > > > > > > + * gstreamer_memory_lifetime_test.cpp - GStreamer memory lifetime test\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \"gstreamer_test.h\"\n> > > > > > > +#include \"test.h\"\n> > > > > > > +\n> > > > > > > +using namespace std;\n> > > > > > > +\n> > > > > > > +class GstreamerMemoryLifetimeTest : public GstreamerTest, public Test\n> > > > > > > +{\n> > > > > > > +public:\n> > > > > > > + GstreamerMemoryLifetimeTest()\n> > > > > > > + : GstreamerTest()\n> > > > > > > + {\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > +protected:\n> > > > > > > + int init() override\n> > > > > > > + {\n> > > > > > > + if (status_ != TestPass)\n> > > > > > > + return status_;\n> > > > > > > +\n> > > > > > > + appsink_ = gst_element_factory_make(\"appsink\", nullptr);\n> > > > > > > + if (!appsink_) {\n> > > > > > > + g_printerr(\"Your installation is missing 'appsink'\\n\");\n> > > > > > > + return TestFail;\n> > > > > > > + }\n> > > > > > > + g_object_ref_sink(appsink_);\n> > > > > > > +\n> > > > > > > + return createPipeline();\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + int run() override\n> > > > > > > + {\n> > > > > > > + /* Build the pipeline */\n> > > > > > > + gst_bin_add_many(GST_BIN(pipeline_), libcameraSrc_, appsink_, nullptr);\n> > > > > > > + if (gst_element_link(libcameraSrc_, appsink_) != TRUE) {\n> > > > > > > + g_printerr(\"Elements could not be linked.\\n\");\n> > > > > > > + return TestFail;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + if (startPipeline() != TestPass)\n> > > > > > > + return TestFail;\n> > > > > \n> > > > > Did not add a comment here, since \"startPipeline()\" should be obvious that we\n> > > > > start the pipeline. It will raise the state to PLAYING state.\n> > > > \n> > > > Yes, startPipeline is obvious enough ;-)\n> > > > \n> > > > > > > +\n> > > > > > > + sample_ = gst_app_sink_try_pull_sample(GST_APP_SINK(appsink_), GST_SECOND * 5);\n> > > > > \n> > > > > This is also a bit trivial, it pulls a sample from the pipeline.\n> > > > \n> > > > Agreed, but what's the impact in lifetime. do we expect a single buffer?\n> > > > I think you're covering it on the comment below though.\n> > > > \n> > > > > > > + gst_element_set_state(pipeline_, GST_STATE_NULL);\n> > > > > \n> > > > > Here though, I should probably add a comment (well before the change to NULL\n> > > > > state).\n> > > > > \n> > > > > /* \n> > > > > * Stop the pipeline without releasing sample_. This ensures that we can hold\n> > > > > * on memory while elements can release all their internal resources as required\n> > > > > * by the NULL state definition.\n> > > > > */\n> > > > \n> > > > Yes, that's what I think the test needs to document.\n> > > \n> > > +1. That's also the part I wasn't sure about.\n> > > \n> > > > > > > +\n> > > > > > > + if (!sample_)\n> > > > > > > + return TestFail;\n> > > > > > \n> > > > > > Is this the part that does the test? I don't really know how this test\n> > > > > > is working.\n> > > > > \n> > > > > This is memory safety. As the doc says:\n> > > > > \n> > > > > https://gstreamer.freedesktop.org/documentation/applib/gstappsink.html?gi-language=c#gst_app_sink_pull_sample\n> > > > > \n> > > > > > Returns ( [transfer: full][nullable]) – a GstSample or NULL ...\n> > > > > \n> > > > > As it annoted nullable, not doing a null check if not safe. I'd, say, it would\n> > > > > be annormal, so failing the test make sense, but its not really the expected\n> > > > > failure spot for when the code was broken.\n> > > \n> > > Should/can it be checked before calling gst_element_set_state(), or is\n> > > the order above important ? sample_ being a normal pointer I assume that\n> > > the call to gst_element_set_state() won't modify it, but the order of\n> > > the code made it feel like there was some magic happening.\n> > \n> > If you prefer, we can duplicate the set_state call, here's the pseudo code of\n> > what can be done, also added a draft of the missing comment.\n> > \n> > sample = pull() \n> > if (!sample) { \n> > set_state(NULL); return failed; \n> > } set_state(NULL);\n> > \n> > /* \n> > * Keep the sample referenced. This way, it will be released \n> > * in cleanup() after the camera manager object. This used to \n> > * lead to a crash as freeing video frames tried to call into \n> > * the freed camera manager. \n> > */ \n> > return success;\n> \n> Normally I would go for the code included in this patch, it's more\n> efficient. In this specific case, as the code is a test case, I think\n> it's important to make it as clear as possible. How about the following\n> (which verifies my good understanding of the test case) ?\n> \n> sample = pull() \n> if (!sample) { \n> set_state(NULL);\n> return failed;\n> }\n> \n> /* \n> * Keep the sample referenced and set the pipeline state to NULL. This\n> * causes the libcamerasrc element to synchronously release resources it\n> * holds. The sample will be released later in cleanup().\n> *\n> * The test case verifies that libcamerasrc keeps alive long enough all\n> * the resources that are needed until memory allocated for frames gets\n> * freed. Any use-after-free will be caught by the test crashing in\n> * cleanup().\n> */ \n> set_state(NULL);\n> return success;\n> \n\n\nI think this is the best option to clarify the intent of the test,\nand document what's happening.\n\n\nReviewed-by: Kieran Bingham \n\nI'll make these changes while applying to get this series merged\nalready, and also fix the test to run first with ExpectedFail which the\ncommit that handles the fix will remove.\n\n--\nKieran\n\n> > The reason being that behaviour is undefined (and document to deadlock most of\n> > the time) if you drop the last reference of a pipeline that is running (any\n> > state above NULL).\n> > \n> > > > Which is the point that spots the code is broken then? I seem to still\n> > > > miss it ?\n> > > > \n> > > > > > Is it pulling a single buffer? Does set_state(GST_STATE_NULL) destroy\n> > > > > > the pipeline?\n> > > > > \n> > > > > No, when moving to STATE_NULL, its is required \n> > > > > \n> > > > > > Any comments we can add here to make it clearer what is actually being\n> > > > > > tested or happening? The code doesn't really tell me what's going on\n> > > > > > under the hood.\n> > > > > \n> > > > > https://gstreamer.freedesktop.org/documentation/application-development/basics/elements.html?gi-language=c#element-states\n> > > > > \n> > > > > > GST_STATE_NULL: this is the default state. No resources are allocated in this\n> > > > > state, so, transitioning to it will free all resources. The element must be in \n> > > > > this state when its refcount reaches 0 and it is freed.\n> > > > > \n> > > > > Let me know if the suggested comment above would do for you.\n> > > > > \n> > > > > > Does the test for !sample_ really mean the buffer still exists and is\n> > > > > > valid? Does the test need to do anything to /access/ the buffer to make\n> > > > > > sure the test proves it succeeded?\n> > > > > > \n> > > > > > I expect it probably does as otherwise you'd have added more - but it's\n> > > > > > just not obvious from the above.\n> > > > > \n> > > > > The bug being that libcamera crash when the memory is released, in this specific\n> > > > > case it crash in cleanup(). But I suspect that we can drop the sample_ member,\n> > > > > and simple manually unref the sample here (avoiding smart pointer, as it would\n> > > > > be equally confusing).\n> > > > > \n> > > > > p.s. Btw, its hard for me to efficiently comment back on 2 months old patch, I\n> > > > > don't remember much about it, I'm just reading the patch to reply here.\n> > > \n> > > Sorry for not replying earlier.\n> > > \n> > > > Sounds like a great reason to make sure we have comments in the code\n> > > > that explain the 'why' :-)\n> > > > \n> > > > I'm fine with whatever comments you believe are appropriate. 'You' two\n> > > > months later are probably the best reviewer here ;-)\n> > > \n> > > Another comment about this patch in particular. We tend to order this\n> > > kind of series with the test coming first, and the fix coming next. This\n> > > allows ensuring that the test fails, and that the fix fixes the issue.\n> > > This guards against cases where the test would be incorrect and wouldn't\n> > > showcase the actual problem, and the fix would also be bogus.\n> > \n> > Sure, as we discussed on IRC, in my other projects we do the total opposite to\n> > avoid making bisection painful. Might be something to revisit later for you.\n> \n> Sure. We could also mark the test as expected to fail, and turn it into\n> expected to succeed in the patch that fixes the issue.\n> \n> > > > > > > +\n> > > > > > > + return TestPass;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void cleanup() override\n> > > > > > > + {\n> > > > \n> > > > Are you saying the test is actually testing the issue /here/ ? If so -\n> > > > can we add a comment explicitly saying that ?\n> > > > \n> > > > Otherwise this is just 'cleanup' code to me. That's why I was looking at\n> > > > the null check of sample_ trying to figure out if that's how you were\n> > > > determining if the test had failed (Becuase that's the only time you\n> > > > return TestFail after the test actually starts).\n> > > > \n> > > > \n> > > > I guess the failure point is that the test will crash, rather than\n> > > > detect a failure. Would that also be noteworthy?\n> > > \n> > > That is worth a comment, yes.\n> > > \n> > > > > > > + g_clear_pointer(&sample_, gst_sample_unref);\n> > > > > > > + g_clear_object(&appsink_);\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > +private:\n> > > > > > > + GstElement *appsink_;\n> > > > > > > + GstSample *sample_;\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +TEST_REGISTER(GstreamerMemoryLifetimeTest)\n> > > > > > > diff --git a/test/gstreamer/meson.build b/test/gstreamer/meson.build\n> > > > > > > index f3ba5a23..70609b88 100644\n> > > > > > > --- a/test/gstreamer/meson.build\n> > > > > > > +++ b/test/gstreamer/meson.build\n> > > > > > > @@ -8,12 +8,14 @@ gstreamer_tests = [\n> > > > > > > {'name': 'single_stream_test', 'sources': ['gstreamer_single_stream_test.cpp']},\n> > > > > > > {'name': 'multi_stream_test', 'sources': ['gstreamer_multi_stream_test.cpp']},\n> > > > > > > {'name': 'device_provider_test', 'sources': ['gstreamer_device_provider_test.cpp']},\n> > > > > > > + {'name': 'memory_lifetime_test', 'sources': ['gstreamer_memory_lifetime_test.cpp']},\n> > > > > > > ]\n> > > > > > > gstreamer_dep = dependency('gstreamer-1.0', required : true)\n> > > > > > > +gstapp_dep = dependency('gstreamer-app-1.0', required : true)\n> > > > > > > \n> > > > > > > foreach test : gstreamer_tests\n> > > > > > > exe = executable(test['name'], test['sources'], 'gstreamer_test.cpp',\n> > > > > > > - dependencies : [libcamera_private, gstreamer_dep],\n> > > > > > > + dependencies : [libcamera_private, gstreamer_dep, gstapp_dep],\n> > > > > > > link_with : test_libraries,\n> > > > > > > include_directories : test_includes_internal)\n> > > > > > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBB12BDB1C\n\tfor ;\n\tThu, 25 Jul 2024 10:48:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B69F66336F;\n\tThu, 25 Jul 2024 12:48:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE8C86199E\n\tfor ;\n\tThu, 25 Jul 2024 12:48:36 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0EF8745B;\n\tThu, 25 Jul 2024 12:47:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jkY+V3bJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721904473;\n\tbh=qznPk7jPnx46xte44Bdo+B2QAPyjBW3Sc/Tl0zj3pNA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jkY+V3bJEJiGf8WJBzwx7pM3adTKGfA6rmD1gEV2+8X0faGHEzrlPhPlrgzcXyTg/\n\tZZrpjj3PjXZX2qkUbMtkiEw1ccBrXC3qY2xb6GEDcX2JN1hTLX928M3ZSGfkzUoUQ5\n\t8nhQJjXp10+pQiDDDAD93aJLYm8YP81GJ8VgxZ50=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240514160216.GH32013@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-4-nicolas@ndufresne.ca>\n\t<171526664530.1857112.14163952053858940810@ping.linuxembedded.co.uk>\n\t\n\t<171527087153.75000.12455649241262473827@ping.linuxembedded.co.uk>\n\t<20240512223504.GU17158@pendragon.ideasonboard.com>\n\t<020c1831147138dd84e81e5b8168d14045024e24.camel@collabora.com>\n\t<20240514160216.GH32013@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 3/3] test: gstreamer: Test memory lifetime","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tNicolas Dufresne ","Date":"Thu, 25 Jul 2024 11:48:33 +0100","Message-ID":"<172190451318.392292.4588062676990013289@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]