[{"id":29502,"web_url":"https://patchwork.libcamera.org/comment/29502/","msgid":"<171526585901.1857112.10327799757776221829@ping.linuxembedded.co.uk>","date":"2024-05-09T14:44:19","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","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:56)\n> From: Nicolas Dufresne \n> \n> With the camera manager, it is not possible to cleanly delete the\n\nDoes this mean 'with' or 'without the camera manager' ?\n\n> FrameBufferAllocator object. Keep the camera manager alive until all the\n> memory object have been released.\n> \n> Fixes Bugzilla issue 211\n> \n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=211\n\n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> 1 file changed, 15 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> index c740b8fc..844bdb17 100644\n> --- a/src/gstreamer/gstlibcameraallocator.cpp\n> +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> * FrameWrap.\n> */\n> GHashTable *pools;\n> + /*\n> + * The camera manager represent that library, which needs to be kept alive\n> + * until all the memory have been released.\n\ns/represent that/represents the/ ?\ns/have/has/\n\nBut those are just nits could fixed while applying.\n\nI recall Laurent saying he had some worries about this series, thinking\nthat there might be a better way to handle keeping the lifetime managed.\n\nBut - This fixes a bug, and is fully green on the CI, and even\nintroduces a test to validate it. So I'd be keen to see this series\nmerged. There's always the opportunity to refactor in the future if\nsomeone desired it.\n\nReviewed-by: Kieran Bingham \n\n> + */\n> + std::shared_ptr *cm_ptr;\n> };\n> \n> G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> \n> delete self->fb_allocator;\n> \n> + /* keep last */\n> + delete self->cm_ptr;\n> +\n> G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> }\n> \n> @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> {\n> auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> nullptr));\n> + gint ret;\n> +\n> + self->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n> + if (ret) {\n> + g_object_unref(self);\n> + return nullptr;\n> + }\n> \n> self->fb_allocator = new FrameBufferAllocator(camera);\n> for (StreamConfiguration &streamCfg : *config_) {\n> Stream *stream = streamCfg.stream();\n> - gint ret;\n> \n> ret = self->fb_allocator->allocate(stream);\n> if (ret == 0)\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 12474C3226\n\tfor ;\n\tThu, 9 May 2024 14:44:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8C206342D;\n\tThu, 9 May 2024 16:44:22 +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 C8725633FA\n\tfor ;\n\tThu, 9 May 2024 16:44:21 +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 17FE2236;\n\tThu, 9 May 2024 16:44:18 +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=\"bgJ7KOLT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715265858;\n\tbh=oA/Z6hQxZl1Oh1TmS9QOAgpXLsSpidlC1FnBsb7prKU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bgJ7KOLTv3CBIyfxUa3xfv1NHbhYKslv2BqXAcdT2bTwwd18pGRddbM3JtFYD72rn\n\tFLCgFWZkYKpFzhnzHJiw4YPfudLb41JcLTXNkCm0KgiHifIHbbNBsFsBEZvK97mLNz\n\tahWTGPJphUcGMlwPKcpxM2FGlVZhUqGIYBz5lm2s=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240305153058.1761020-2-nicolas@ndufresne.ca>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","From":"Kieran Bingham ","Cc":"Nicolas Dufresne ","To":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 May 2024 15:44:19 +0100","Message-ID":"<171526585901.1857112.10327799757776221829@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":29505,"web_url":"https://patchwork.libcamera.org/comment/29505/","msgid":"","date":"2024-05-09T15:05:32","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nthanks for the review,\n\nLe jeudi 09 mai 2024 à 15:44 +0100, Kieran Bingham a écrit :\n> Quoting Nicolas Dufresne (2024-03-05 15:30:56)\n> > From: Nicolas Dufresne \n> > \n> > With the camera manager, it is not possible to cleanly delete the\n> \n> Does this mean 'with' or 'without the camera manager' ?\n\nWithout, sorry.\n\n> \n> > FrameBufferAllocator object. Keep the camera manager alive until all the\n> > memory object have been released.\n> > \n> > Fixes Bugzilla issue 211\n> > \n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=211\n\nAck. (did we document our tagging somewhere ?)\n\n> \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index c740b8fc..844bdb17 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> > * FrameWrap.\n> > */\n> > GHashTable *pools;\n> > + /*\n> > + * The camera manager represent that library, which needs to be kept alive\n> > + * until all the memory have been released.\n> \n> s/represent that/represents the/ ?\n> s/have/has/\n> \n> But those are just nits could fixed while applying.\n\nAck.\n\n> \n> I recall Laurent saying he had some worries about this series, thinking\n> that there might be a better way to handle keeping the lifetime managed.\n\nSure, though my opinion is that project that ships should accept valid work\naround for serious issues that will requires month to be fixed properly.\n\n> \n> But - This fixes a bug, and is fully green on the CI, and even\n> introduces a test to validate it. So I'd be keen to see this series\n> merged. There's always the opportunity to refactor in the future if\n> someone desired it.\n\nIndeed, the idea with the test, is that you can later revert this patch but keep\nthe test. Since the test will be green if you figure-out another way to fix it.\nPerhaps it could have been a separate patch for making this easier. I simply\ndidn't think about that earlier.\n\nLet me know, I can resend if need be.\n\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + */\n> > + std::shared_ptr *cm_ptr;\n> > };\n> > \n> > G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> > \n> > delete self->fb_allocator;\n> > \n> > + /* keep last */\n> > + delete self->cm_ptr;\n> > +\n> > G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > }\n> > \n> > @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > {\n> > auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > nullptr));\n> > + gint ret;\n> > +\n> > + self->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n> > + if (ret) {\n> > + g_object_unref(self);\n> > + return nullptr;\n> > + }\n> > \n> > self->fb_allocator = new FrameBufferAllocator(camera);\n> > for (StreamConfiguration &streamCfg : *config_) {\n> > Stream *stream = streamCfg.stream();\n> > - gint ret;\n> > \n> > ret = self->fb_allocator->allocate(stream);\n> > if (ret == 0)\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 A4197BDE6B\n\tfor ;\n\tThu, 9 May 2024 15:05:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 955BA63464;\n\tThu, 9 May 2024 17:05:38 +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 62E116345F\n\tfor ;\n\tThu, 9 May 2024 17:05:37 +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 B140F37811CD; \n\tThu, 9 May 2024 15:05:36 +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=\"rjktRgdC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1715267137;\n\tbh=DJbnAR9AqHizyj6CK/b3WJb7FDfk5QL0qjchSGzriNo=;\n\th=Subject:From:To:Date:In-Reply-To:References:From;\n\tb=rjktRgdCvsjjb8gCr8dYRl6eM6WQnp/BIY5H9PxvB/z4HB4MQgjaWI0ZKwYfS4r3d\n\txzYtjzwQHHRwDdb7SO/v2oanxiLf2MfkJxh/chVkiVqehNc/6rlIBdO8qE2RZI4sK0\n\tVrQuxVoEkNPIz2ZrcggvSOio/JHX6GGuufbbONwAu07GP5JoajMrSTY7OGtLsx2jGh\n\tHMvI4bJBr0wJ1h4s0iEgtkdsWJINy/6xVaJuRskg1Sn+r19xj1SVpkcyboi4T4YExq\n\tV646b/W9FjkeEexhQrgfKEnxVcd8dYaPwC1lamDsoQUfUdRDh3hG4ZO7sdUAj15toW\n\tRGh9bH4kxcNCg==","Message-ID":"","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","From":"Nicolas Dufresne ","To":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 May 2024 11:05:32 -0400","In-Reply-To":"<171526585901.1857112.10327799757776221829@ping.linuxembedded.co.uk>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>\n\t<171526585901.1857112.10327799757776221829@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":29527,"web_url":"https://patchwork.libcamera.org/comment/29527/","msgid":"<20240514160818.GI32013@pendragon.ideasonboard.com>","date":"2024-05-14T16:08:18","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Tue, Mar 05, 2024 at 10:30:56AM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> With the camera manager, it is not possible to cleanly delete the\n\nDid you mean s/With/Without/ ?\n\n> FrameBufferAllocator object. Keep the camera manager alive until all the\n> memory object have been released.\n> \n> Fixes Bugzilla issue 211\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> 1 file changed, 15 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> index c740b8fc..844bdb17 100644\n> --- a/src/gstreamer/gstlibcameraallocator.cpp\n> +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> \t * FrameWrap.\n> \t */\n> \tGHashTable *pools;\n> +\t/*\n> +\t * The camera manager represent that library, which needs to be kept alive\n> +\t * until all the memory have been released.\n> +\t */\n> +\tstd::shared_ptr *cm_ptr;\n\nThis doesn't have to be a pointer, it can be\n\n\tstd::shared_ptr cm;\n\n> };\n> \n> G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> \n> \tdelete self->fb_allocator;\n> \n> +\t/* keep last */\n> +\tdelete self->cm_ptr;\n> +\n\nThis would become\n\n\t/* Keep last. */\n\tself->cm.reset();\n\n> \tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> }\n> \n> @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> {\n> \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> \t\t\t\t\t\t\t nullptr));\n> +\tgint ret;\n> +\n> +\tself->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n\nAnd here,\n\n\tself->cm = gst_libcamera_get_camera_manager(ret);\n\n> +\tif (ret) {\n> +\t\tg_object_unref(self);\n> +\t\treturn nullptr;\n> +\t}\n> \n> \tself->fb_allocator = new FrameBufferAllocator(camera);\n> \tfor (StreamConfiguration &streamCfg : *config_) {\n> \t\tStream *stream = streamCfg.stream();\n> -\t\tgint ret;\n> \n> \t\tret = self->fb_allocator->allocate(stream);\n> \t\tif (ret == 0)","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 D8781BD78E\n\tfor ;\n\tTue, 14 May 2024 16:08:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAD586346B;\n\tTue, 14 May 2024 18:08:27 +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 48D1763469\n\tfor ;\n\tTue, 14 May 2024 18:08: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 D2F1A27C;\n\tTue, 14 May 2024 18:08:18 +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=\"dwsH3YUj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715702899;\n\tbh=68Pou/CmwEU0ovpcbFD+IOSNb+23Pg5C2S0tdB/Zt9w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dwsH3YUj5jskdclkTuYLTdM2yBfFzTdU/MLmRAnzY8z7BRCLHePySJ0Vxuv/sKIPC\n\tGxEsBC9eLJguMuXjx4RAyeq9fBl3VTi0I/x/NvFW5RAPkRjuxBGZMdhjFMlIXuKYlw\n\t35SPAgRZ1HWYHr9tjtGUb1OoTQYXySGbBbxYJ4Pc=","Date":"Tue, 14 May 2024 19:08:18 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","Message-ID":"<20240514160818.GI32013@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240305153058.1761020-2-nicolas@ndufresne.ca>","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":29528,"web_url":"https://patchwork.libcamera.org/comment/29528/","msgid":"<1eea18672f53184858ef6abe2e9d4a62e61c2b0d.camel@collabora.com>","date":"2024-05-14T16:53:51","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Laurent,\n\nLe mardi 14 mai 2024 à 19:08 +0300, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 05, 2024 at 10:30:56AM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > With the camera manager, it is not possible to cleanly delete the\n> \n> Did you mean s/With/Without/ ?\n\nWithout, was already mentioned before.\n\n> \n> > FrameBufferAllocator object. Keep the camera manager alive until all the\n> > memory object have been released.\n> > \n> > Fixes Bugzilla issue 211\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index c740b8fc..844bdb17 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> > \t * FrameWrap.\n> > \t */\n> > \tGHashTable *pools;\n> > +\t/*\n> > +\t * The camera manager represent that library, which needs to be kept alive\n> > +\t * until all the memory have been released.\n> > +\t */\n> > +\tstd::shared_ptr *cm_ptr;\n> \n> This doesn't have to be a pointer, it can be\n\nNo, it can't, that C structure is alloc/freed by glib, as a side effect will be\nmissing a destructor call to drop the shared pointer reference. In the\nGstElement code, that is why we have a C++ class, the \"state\". It makes\neverything more convenient. But until we have more then once C++ data member its\nnot worth it.\n\nregards,\nNicolas\n\n> \n> \tstd::shared_ptr cm;\n> \n> > };\n> > \n> > G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> > \n> > \tdelete self->fb_allocator;\n> > \n> > +\t/* keep last */\n> > +\tdelete self->cm_ptr;\n> > +\n> \n> This would become\n> \n> \t/* Keep last. */\n> \tself->cm.reset();\n> \n> > \tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > }\n> > \n> > @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > {\n> > \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > \t\t\t\t\t\t\t nullptr));\n> > +\tgint ret;\n> > +\n> > +\tself->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n> \n> And here,\n> \n> \tself->cm = gst_libcamera_get_camera_manager(ret);\n> \n> > +\tif (ret) {\n> > +\t\tg_object_unref(self);\n> > +\t\treturn nullptr;\n> > +\t}\n> > \n> > \tself->fb_allocator = new FrameBufferAllocator(camera);\n> > \tfor (StreamConfiguration &streamCfg : *config_) {\n> > \t\tStream *stream = streamCfg.stream();\n> > -\t\tgint ret;\n> > \n> > \t\tret = self->fb_allocator->allocate(stream);\n> > \t\tif (ret == 0)\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 EDEE7BDE6B\n\tfor ;\n\tTue, 14 May 2024 16:53:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E69676347E;\n\tTue, 14 May 2024 18:53:58 +0200 (CEST)","from madrid.collaboradmins.com (madrid.collaboradmins.com\n\t[46.235.227.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FB2B63469\n\tfor ;\n\tTue, 14 May 2024 18:53:58 +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 554B23780A0B; \n\tTue, 14 May 2024 16:53:57 +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=\"jdQpahiM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1715705637;\n\tbh=3haDWNuksHO4Oq72x6ZwooRftUMx45e4EloNsRLxiDc=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=jdQpahiM3maR5CEOd5hb4OyHcMCQcNajoVKu1jRPjkJHWHwOQ8CHyOl8DNLJYVyvc\n\tEzzJ5yeTw2FD+43tQl9b3IATEdT9/P8F8PoIADD/5N7I+k6u1tVjwoEJzv5/O73OGs\n\tuqTm7bnDUcwMho0GOj15eDgWgNNSR6V4s9/RxxnuYDGaT4r5n7dmVj+un8hD/VSm3F\n\tcArgsMq+SH1nmgW977NyGzOMXk/UxOUuIoO8BGl6gktlBQcfL2qQtuOMLReZbPg03H\n\t9KFpBJa7e7+jke2zlAtBI7aqeepWemhJ+sYI6UAwJDrlszloFlzRXndAf9K8yjp223\n\t38BoOwsagKjew==","Message-ID":"<1eea18672f53184858ef6abe2e9d4a62e61c2b0d.camel@collabora.com>","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 14 May 2024 12:53:51 -0400","In-Reply-To":"<20240514160818.GI32013@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>\n\t<20240514160818.GI32013@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":29529,"web_url":"https://patchwork.libcamera.org/comment/29529/","msgid":"<20240514184044.GA11473@pendragon.ideasonboard.com>","date":"2024-05-14T18:40:44","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Tue, May 14, 2024 at 12:53:51PM -0400, Nicolas Dufresne wrote:\n> Le mardi 14 mai 2024 à 19:08 +0300, Laurent Pinchart a écrit :\n> > On Tue, Mar 05, 2024 at 10:30:56AM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne \n> > > \n> > > With the camera manager, it is not possible to cleanly delete the\n> > \n> > Did you mean s/With/Without/ ?\n> \n> Without, was already mentioned before.\n> \n> > > FrameBufferAllocator object. Keep the camera manager alive until all the\n> > > memory object have been released.\n> > > \n> > > Fixes Bugzilla issue 211\n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> > > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > > index c740b8fc..844bdb17 100644\n> > > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > > @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> > > \t * FrameWrap.\n> > > \t */\n> > > \tGHashTable *pools;\n> > > +\t/*\n> > > +\t * The camera manager represent that library, which needs to be kept alive\n> > > +\t * until all the memory have been released.\n> > > +\t */\n> > > +\tstd::shared_ptr *cm_ptr;\n> > \n> > This doesn't have to be a pointer, it can be\n> \n> No, it can't, that C structure is alloc/freed by glib, as a side effect will be\n> missing a destructor call to drop the shared pointer reference. In the\n> GstElement code, that is why we have a C++ class, the \"state\". It makes\n> everything more convenient. But until we have more then once C++ data member its\n> not worth it.\n\nYou're right. Could you record this in the commit message ?\n\n> > \tstd::shared_ptr cm;\n> > \n> > > };\n> > > \n> > > G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> > > \n> > > \tdelete self->fb_allocator;\n> > > \n> > > +\t/* keep last */\n> > > +\tdelete self->cm_ptr;\n> > > +\n> > \n> > This would become\n> > \n> > \t/* Keep last. */\n> > \tself->cm.reset();\n> > \n> > > \tG_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > > }\n> > > \n> > > @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > > {\n> > > \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > > \t\t\t\t\t\t\t nullptr));\n> > > +\tgint ret;\n> > > +\n> > > +\tself->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n> > \n> > And here,\n> > \n> > \tself->cm = gst_libcamera_get_camera_manager(ret);\n> > \n> > > +\tif (ret) {\n> > > +\t\tg_object_unref(self);\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > \n> > > \tself->fb_allocator = new FrameBufferAllocator(camera);\n> > > \tfor (StreamConfiguration &streamCfg : *config_) {\n> > > \t\tStream *stream = streamCfg.stream();\n> > > -\t\tgint ret;\n> > > \n> > > \t\tret = self->fb_allocator->allocate(stream);\n> > > \t\tif (ret == 0)","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 6D7BEBD78E\n\tfor ;\n\tTue, 14 May 2024 18:40:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F07C6346B;\n\tTue, 14 May 2024 20:40:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F06263469\n\tfor ;\n\tTue, 14 May 2024 20:40:53 +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 973A04D0;\n\tTue, 14 May 2024 20:40:45 +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=\"TDBO+VOQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715712045;\n\tbh=rJC6glBrmnj3AqB+crK3VZ1YsYyUjXkj7HGwE4/ESoc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TDBO+VOQ5MrE6hc5JufXtypt+NpXnVK/4jX0hYnWwmizdp87HOM2rRDmf9JtWK7Ry\n\tXnde/o5UOgO5VP9VcsYlETzrRWoOYv8VeVqqcSBKcVNwJL4y3ZJ9dXzx5XSSur9Rab\n\tF6U/OHubKCbD0ktYYNzG+G2V5w5QMFZ3Y1k9u7es=","Date":"Tue, 14 May 2024 21:40:44 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","Message-ID":"<20240514184044.GA11473@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>\n\t<20240514160818.GI32013@pendragon.ideasonboard.com>\n\t<1eea18672f53184858ef6abe2e9d4a62e61c2b0d.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1eea18672f53184858ef6abe2e9d4a62e61c2b0d.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":29534,"web_url":"https://patchwork.libcamera.org/comment/29534/","msgid":"<20240514202550.GL32013@pendragon.ideasonboard.com>","date":"2024-05-14T20:25:50","subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","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 03:44:19PM +0100, Kieran Bingham wrote:\n> Quoting Nicolas Dufresne (2024-03-05 15:30:56)\n> > From: Nicolas Dufresne \n> > \n> > With the camera manager, it is not possible to cleanly delete the\n> \n> Does this mean 'with' or 'without the camera manager' ?\n> \n> > FrameBufferAllocator object. Keep the camera manager alive until all the\n> > memory object have been released.\n> > \n> > Fixes Bugzilla issue 211\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=211\n> \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 16 +++++++++++++++-\n> > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index c740b8fc..844bdb17 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -100,6 +100,11 @@ struct _GstLibcameraAllocator {\n> > * FrameWrap.\n> > */\n> > GHashTable *pools;\n> > + /*\n> > + * The camera manager represent that library, which needs to be kept alive\n> > + * until all the memory have been released.\n> \n> s/represent that/represents the/ ?\n> s/have/has/\n> \n> But those are just nits could fixed while applying.\n> \n> I recall Laurent saying he had some worries about this series, thinking\n> that there might be a better way to handle keeping the lifetime managed.\n\nI (finally) had a look at that today, and I think this patch is the way\nto go, at least for now. We should redesign the FrameBufferAllocator at\nsome point, the current API isn't great, but for the time being, this is\nthe best way forward.\n\n> But - This fixes a bug, and is fully green on the CI, and even\n> introduces a test to validate it. So I'd be keen to see this series\n> merged. There's always the opportunity to refactor in the future if\n> someone desired it.\n> \n> Reviewed-by: Kieran Bingham \n>\n> > + */\n> > + std::shared_ptr *cm_ptr;\n> > };\n> > \n> > G_DEFINE_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,\n> > @@ -173,6 +178,9 @@ gst_libcamera_allocator_finalize(GObject *object)\n> > \n> > delete self->fb_allocator;\n> > \n> > + /* keep last */\n\ns/keep last/Keep last./\n\nReviewed-by: Laurent Pinchart \n\n> > + delete self->cm_ptr;\n> > +\n> > G_OBJECT_CLASS(gst_libcamera_allocator_parent_class)->finalize(object);\n> > }\n> > \n> > @@ -193,11 +201,17 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > {\n> > auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > nullptr));\n> > + gint ret;\n> > +\n> > + self->cm_ptr = new std::shared_ptr(gst_libcamera_get_camera_manager(ret));\n> > + if (ret) {\n> > + g_object_unref(self);\n> > + return nullptr;\n> > + }\n> > \n> > self->fb_allocator = new FrameBufferAllocator(camera);\n> > for (StreamConfiguration &streamCfg : *config_) {\n> > Stream *stream = streamCfg.stream();\n> > - gint ret;\n> > \n> > ret = self->fb_allocator->allocate(stream);\n> > if (ret == 0)","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 19519BDE6B\n\tfor ;\n\tTue, 14 May 2024 20:26:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4057E6346B;\n\tTue, 14 May 2024 22:25:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A81763469\n\tfor ;\n\tTue, 14 May 2024 22:25:58 +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 8B86727C;\n\tTue, 14 May 2024 22:25: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=\"JJxC0Con\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715718350;\n\tbh=1001C8kfkWV6a6AetfG5DwaH/nJAQvEqc34lP2v7vDQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JJxC0ConrttcTRY+jEBf2dnd0c76+19zRUsmnEmm/ZLHGOzcctfB54GdPQ+svPhdT\n\tJwnCHmdW37MkyftHi41L6l1sj0JrkxZvmgELU+PBp9sLw1VLfB9Q1H/Vb+R8bK0gJS\n\tlDo3TChPVGdc+QhA4W8KAOcijFsLuziFJqThNZ/c=","Date":"Tue, 14 May 2024 23:25:50 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Subject":"Re: [PATCH v2 1/3] gstreamer: allocator: Ensure camera manager stay\n\talive","Message-ID":"<20240514202550.GL32013@pendragon.ideasonboard.com>","References":"<20240305153058.1761020-1-nicolas@ndufresne.ca>\n\t<20240305153058.1761020-2-nicolas@ndufresne.ca>\n\t<171526585901.1857112.10327799757776221829@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171526585901.1857112.10327799757776221829@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\" "}}]