[{"id":29530,"web_url":"https://patchwork.libcamera.org/comment/29530/","msgid":"<20240514185048.GJ32013@pendragon.ideasonboard.com>","date":"2024-05-14T18:50:48","subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, May 14, 2024 at 05:45:39PM +0000, Barnabás Pőcze wrote:\n> The string returned by `gst_video_colorimetry_to_string()`\n> has to be freed,\n\nThis is right.\n\n> this was missing.\n\nBut are you sure about this ? The allocated colorimetry_str is passed to\ngst_structure_set(), which I *think* doesn't copy the string but takes\nownership of it.\n\n> Fixes: fc9783acc6083a (\"gstreamer: Provide colorimetry <> ColorSpace mappings\")\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index e163ce41..ec4da435 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -385,7 +385,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \n> \tif (stream_cfg.colorSpace) {\n> \t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> -\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> +\t\tg_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> \n> \t\tif (colorimetry_str)\n> \t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);","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 F33D5BDE6B\n\tfor ;\n\tTue, 14 May 2024 18:50:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A8ED6347E;\n\tTue, 14 May 2024 20:50:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D6F463469\n\tfor ;\n\tTue, 14 May 2024 20:50:56 +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 C40EB4D0;\n\tTue, 14 May 2024 20:50:48 +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=\"OU4xN23t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715712649;\n\tbh=kbyJxX28g3eo64epO/ncqeCLOIYy9yk1wlFTitPeRPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OU4xN23tnSRiS5BLZX27PhLTbons/x2TLv46WTKXQSexX+/qozuhLTnWXDhjBvnkB\n\tRZ8t51CVkKlP0lFvtwTd9XBcbRzYc+ILyJe5z8KHNT+ptYrSnjV9BD7EYHcZQMvvwa\n\tz/mcL9v+4HsntdG8avOK+W+jfaTY1qKNqKvxBvfs=","Date":"Tue, 14 May 2024 21:50:48 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","Message-ID":"<20240514185048.GJ32013@pendragon.ideasonboard.com>","References":"<20240514174538.195350-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20240514174538.195350-1-pobrn@protonmail.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":29532,"web_url":"https://patchwork.libcamera.org/comment/29532/","msgid":"","date":"2024-05-14T19:18:50","subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nLe mardi 14 mai 2024 à 21:50 +0300, Laurent Pinchart a écrit :\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, May 14, 2024 at 05:45:39PM +0000, Barnabás Pőcze wrote:\n> > The string returned by `gst_video_colorimetry_to_string()`\n> > has to be freed,\n> \n> This is right.\n> \n> > this was missing.\n> \n> But are you sure about this ? The allocated colorimetry_str is passed to\n> gst_structure_set(), which I *think* doesn't copy the string but takes\n> ownership of it.\n\nIn glib, default transfer method is \"none\", which means the caller keeps\nownership. gst_structure_set() has no annotation thus it implies that it will\nhave to copy foreign strings. In general, glib API that takes ownership of a\nstring would use the term \"take\". \n\n> \n> > Fixes: fc9783acc6083a (\"gstreamer: Provide colorimetry <> ColorSpace mappings\")\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index e163ce41..ec4da435 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -385,7 +385,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> > \n> > \tif (stream_cfg.colorSpace) {\n> > \t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > -\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > +\t\tg_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > \n> > \t\tif (colorimetry_str)\n> > \t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n\nReviewed-by: Nicolas Dufresne \n\nNote that we should stop hand writing GstCaps/GstStructure, it makes the\nlibcamerasrc produce incomplete Raw video caps. This my mistake, I apology.\nInstead we should fill a GstVideoInfo and make the caps using\ngst_video_info_to_caps(). The obvious benefit is that we no longer have to do\nthat GstVideoColorimetry to string conversion.\n\nSo we should do, StreamConfiguration -> GstVideoInfo -> caps. This will fix many\nissues we are having these days.\n\n:-D\nNicolas","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 B4160BDE6B\n\tfor ;\n\tTue, 14 May 2024 19:19:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2EEC6347E;\n\tTue, 14 May 2024 21:18:59 +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 0D0DE63469\n\tfor ;\n\tTue, 14 May 2024 21:18: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 EF42F37810C3; \n\tTue, 14 May 2024 19:18:56 +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=\"LqkwIZ8k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1715714337;\n\tbh=PN+OdsrKRYwh6fAtsc0J/GFwsbKm6+T5/QljFOVX2Ro=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=LqkwIZ8kfYEGEq31OxLLhCnMNS71Ho3c9dCHgI2mdigXD1903WeUCm+xbMy8QkA2C\n\tKagNRVzi8akhxjbvqgkcs9Zqp4bPZspecT0vL2az/sNMFHibwbf3u4MmiO2kpA1j/5\n\tAHFaBiEErA2jqCrK20J9Zq39RvuuLwOmNQmLrGv1+xJbTVB0Yaf7ec1qgJRQsCHDA7\n\tP9U3OLV5BA3M7P9qvNsAh4Ij8vSMAhAdPwWwZHItnFRmhXnyA16HiKZMsa+rZlGGNI\n\t8DBNMKDO/M57X5MNT4Nkn3c/804/kq5kZ2sdyhprPuyfm5FtantBI94ydLSIlXh2OZ\n\tbKfDB+iV+e4Ug==","Message-ID":"","Subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","From":"Nicolas Dufresne ","To":"Laurent Pinchart , =?utf-8?q?Barna?=\n\t=?utf-8?b?YsOhcyBQxZFjemU=?= ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 14 May 2024 15:18:50 -0400","In-Reply-To":"<20240514185048.GJ32013@pendragon.ideasonboard.com>","References":"<20240514174538.195350-1-pobrn@protonmail.com>\n\t<20240514185048.GJ32013@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":29533,"web_url":"https://patchwork.libcamera.org/comment/29533/","msgid":"<20240514195737.GK32013@pendragon.ideasonboard.com>","date":"2024-05-14T19:57:37","subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, May 14, 2024 at 03:18:50PM -0400, Nicolas Dufresne wrote:\n> Hi,\n> \n> Le mardi 14 mai 2024 à 21:50 +0300, Laurent Pinchart a écrit :\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, May 14, 2024 at 05:45:39PM +0000, Barnabás Pőcze wrote:\n> > > The string returned by `gst_video_colorimetry_to_string()`\n> > > has to be freed,\n> > \n> > This is right.\n> > \n> > > this was missing.\n> > \n> > But are you sure about this ? The allocated colorimetry_str is passed to\n> > gst_structure_set(), which I *think* doesn't copy the string but takes\n> > ownership of it.\n> \n> In glib, default transfer method is \"none\", which means the caller keeps\n> ownership. gst_structure_set() has no annotation thus it implies that it will\n> have to copy foreign strings. In general, glib API that takes ownership of a\n> string would use the term \"take\". \n\nThank you for the explanation. I had a closer look at the code (for my\nown education) and it appears to match.\n\n> > > Fixes: fc9783acc6083a (\"gstreamer: Provide colorimetry <> ColorSpace mappings\")\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > index e163ce41..ec4da435 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > @@ -385,7 +385,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> > > \n> > > \tif (stream_cfg.colorSpace) {\n> > > \t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > > -\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > > +\t\tg_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > > \n> > > \t\tif (colorimetry_str)\n> > > \t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> \n> Reviewed-by: Nicolas Dufresne \n\nReviewed-by: Laurent Pinchart \n\n> Note that we should stop hand writing GstCaps/GstStructure, it makes the\n> libcamerasrc produce incomplete Raw video caps. This my mistake, I apology.\n> Instead we should fill a GstVideoInfo and make the caps using\n> gst_video_info_to_caps(). The obvious benefit is that we no longer have to do\n> that GstVideoColorimetry to string conversion.\n> \n> So we should do, StreamConfiguration -> GstVideoInfo -> caps. This will fix many\n> issues we are having these days.\n\nPatches are welcome :-)","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 7D088BD78E\n\tfor ;\n\tTue, 14 May 2024 19:57:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A8506347E;\n\tTue, 14 May 2024 21:57:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 29C7C63469\n\tfor ;\n\tTue, 14 May 2024 21:57:45 +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 AD33022A;\n\tTue, 14 May 2024 21:57:37 +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=\"IPSKpJna\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715716657;\n\tbh=S+I8Nrk3bEc2ov1QMhhUhWg/oZbWBeslRKF/XLVKRiY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IPSKpJnabvXR0AsnJZ6RoMB+K5lHML7oJS/KrtVPITAb8URZJotSzXJHpitXs6hCy\n\tfxIxZJ2A+l7VhdLo/5KVJ1MuFttagtxwOcAgdsn1R5O9uMJQVOYBjLLq2tCl5bDTNh\n\ty0Zr/9WtbXMgYwIl3aRdYqXUJhh4ZMrsqUQNOYeg=","Date":"Tue, 14 May 2024 22:57:37 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] gstreamer: Fix string memory leak","Message-ID":"<20240514195737.GK32013@pendragon.ideasonboard.com>","References":"<20240514174538.195350-1-pobrn@protonmail.com>\n\t<20240514185048.GJ32013@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]