[{"id":21694,"web_url":"https://patchwork.libcamera.org/comment/21694/","msgid":"<163904435684.2066819.9235024135570227528@Monstersaurus>","date":"2021-12-09T10:05:56","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n> pybind11 needs a public destructor for Camera to be able to manage the\n> shared_ptr. It's not clear why this is needed, and can it be fixed in\n> pybind11.\n> \n\n:-( The 'HACK' here seems to be the only thing that would count as a\nblocker to integration.\n\nI wonder if this is really a HACK or if we just need to explain that the\ndestructor needs to be public to support the python bindings, but I\nmyself don't understand the reasoning itself.\n\nIs the python code the only instance where we create a shared_ptr for\nthe Camera? If that's true - then perhaps this patch should actually be\nsquashed with the one that returns the Camera as a shared_ptr...\n\n--\nKieran\n\n\n> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>\n> ---\n>  include/libcamera/camera.h | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index a7759ccb..88a61ff5 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -104,12 +104,12 @@ public:\n>         int start(const ControlList *controls = nullptr);\n>         int stop();\n>  \n> +       ~Camera();\n>  private:\n>         LIBCAMERA_DISABLE_COPY(Camera)\n>  \n>         Camera(std::unique_ptr<Private> d, const std::string &id,\n>                const std::set<Stream *> &streams);\n> -       ~Camera();\n>  \n>         friend class PipelineHandler;\n>         void disconnect();\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C4D3ABDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:06:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1279F60876;\n\tThu,  9 Dec 2021 11:06:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C802B60224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:05:59 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 68EED501;\n\tThu,  9 Dec 2021 11:05:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"I6VBgQ4y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639044359;\n\tbh=EahDrvvqqkjWemQ1rfINnyQeQx1jkKSx0o0enY23MTA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=I6VBgQ4y13eohOMhLvZlyB0dUltg//V/XVMhSxzdu3h1Fx+2MiZuxGYfdVT45L2Gh\n\thvG3UE3rBQJ+cXC1leRPYIRJxFoSRsy86lMUursOwIZIuMHpn1rh5enwdzYI39r/9i\n\t5MyWe4+H+JmObMOAgvXRM7rd00lLqygo/G0Nco0w=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 Dec 2021 10:05:56 +0000","Message-ID":"<163904435684.2066819.9235024135570227528@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21697,"web_url":"https://patchwork.libcamera.org/comment/21697/","msgid":"<163904460909.2066819.16869224909207480087@Monstersaurus>","date":"2021-12-09T10:10:09","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2021-12-09 10:05:56)\n> Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n> > pybind11 needs a public destructor for Camera to be able to manage the\n> > shared_ptr. It's not clear why this is needed, and can it be fixed in\n> > pybind11.\n\nOk - so now I thought about it, it's clear why it's needed.\nThe shared_ptr means that it holds the reference until the 'end' and\ntherefore might be the cause of calling the destructor, therefore it\nneeds to be public...\n\nSo this really is part of the 'returning a shared_ptr to Camera in the\npublic API', and should be in the next patch.\n\nEither that, or the Camera just gets returned as a non-shared pointer.\n--\nKieran\n\n\n> > \n> \n> :-( The 'HACK' here seems to be the only thing that would count as a\n> blocker to integration.\n> \n> I wonder if this is really a HACK or if we just need to explain that the\n> destructor needs to be public to support the python bindings, but I\n> myself don't understand the reasoning itself.\n> \n> Is the python code the only instance where we create a shared_ptr for\n> the Camera? If that's true - then perhaps this patch should actually be\n> squashed with the one that returns the Camera as a shared_ptr...\n> \n> --\n> Kieran\n> \n> \n> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>\n> > ---\n> >  include/libcamera/camera.h | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index a7759ccb..88a61ff5 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -104,12 +104,12 @@ public:\n> >         int start(const ControlList *controls = nullptr);\n> >         int stop();\n> >  \n> > +       ~Camera();\n> >  private:\n> >         LIBCAMERA_DISABLE_COPY(Camera)\n> >  \n> >         Camera(std::unique_ptr<Private> d, const std::string &id,\n> >                const std::set<Stream *> &streams);\n> > -       ~Camera();\n> >  \n> >         friend class PipelineHandler;\n> >         void disconnect();\n> > -- \n> > 2.25.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 61FAEBDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:10:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E16E60876;\n\tThu,  9 Dec 2021 11:10:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DBF460224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:10:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02160501;\n\tThu,  9 Dec 2021 11:10:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iLygHSk7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639044611;\n\tbh=DajWVIiZ5alPSaQTp7zjDvN5z3RRpcYpPeyPHddkzWU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=iLygHSk718DlgjTUFInWMFymNyVLuqCry5f2qo0oRrWZpB5ulRR8jdIgb1/IRVDGI\n\tCvZ/rzahCdoa9aS8u6/926KkznhzS9MyDZYrQ3YVkaIBSKQbxMtsGyUg3rlYsIq7gN\n\tpwppJeBonTeXAzBg5xpWI3AvgoFNwNrv48pJvK20=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163904435684.2066819.9235024135570227528@Monstersaurus>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 Dec 2021 10:10:09 +0000","Message-ID":"<163904460909.2066819.16869224909207480087@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21699,"web_url":"https://patchwork.libcamera.org/comment/21699/","msgid":"<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>","date":"2021-12-09T10:22:16","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 09/12/2021 12:05, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n>> pybind11 needs a public destructor for Camera to be able to manage the\n>> shared_ptr. It's not clear why this is needed, and can it be fixed in\n>> pybind11.\n>>\n> \n> :-( The 'HACK' here seems to be the only thing that would count as a\n> blocker to integration.\n> \n> I wonder if this is really a HACK or if we just need to explain that the\n> destructor needs to be public to support the python bindings, but I\n> myself don't understand the reasoning itself.\n> \n> Is the python code the only instance where we create a shared_ptr for\n> the Camera? If that's true - then perhaps this patch should actually be\n> squashed with the one that returns the Camera as a shared_ptr...\n\nThis is not related to the patch 2.\n\npybind11 needs a holder type that is used to manage the references to \nthe C++ instances. By default it's unique_ptr, but can be defined. We \nuse shared_ptr for many classes \n(https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n\nI think normally it makes sense that the destructor must be public: if \npybind11 wrapper code can create the instances, it can also destruct \nthem. Here we don't create Cameras, nor destruct them, but the related \ncode is still created, causing the build error.\n\nI believe it should be possible to fix this in pybind11, by making the \ntemplate code to detect these kind of classes and skip the destructor \ncode. Maybe. Somehow. I have no idea how =).\n\n  Tomi","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 25DAFBDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:22:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4615960882;\n\tThu,  9 Dec 2021 11:22:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F9A260224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:22:19 +0100 (CET)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E4CA9501;\n\tThu,  9 Dec 2021 11:22:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AnWm1Qoo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639045339;\n\tbh=Fu0iIRYoJWJBmMzqTI5AAmxfDfz7ffrWMmfeVVJL25Q=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=AnWm1QooBsWtT9DhQYReFApr4vCY/lphGzZrgTYUNH/Vq5Wp85Vfd4LwN//2dUuKy\n\t1G87g/Rxq2GC8fkCzDPSLK51JgHl2wB6X/cQEMW0Upgq4GoRnVUW9a6Ty3h9uZE/Kk\n\t8StA2CUB1aX8wTMJxy0TAsR5lp1R+kR1ivMkHFCU=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>","From":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>","Message-ID":"<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>","Date":"Thu, 9 Dec 2021 12:22:16 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"<163904435684.2066819.9235024135570227528@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21703,"web_url":"https://patchwork.libcamera.org/comment/21703/","msgid":"<163904609199.2066819.9796248885725077869@Monstersaurus>","date":"2021-12-09T10:34:51","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Tomi Valkeinen (2021-12-09 10:22:16)\n> On 09/12/2021 12:05, Kieran Bingham wrote:\n> > Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n> >> pybind11 needs a public destructor for Camera to be able to manage the\n> >> shared_ptr. It's not clear why this is needed, and can it be fixed in\n> >> pybind11.\n> >>\n> > \n> > :-( The 'HACK' here seems to be the only thing that would count as a\n> > blocker to integration.\n> > \n> > I wonder if this is really a HACK or if we just need to explain that the\n> > destructor needs to be public to support the python bindings, but I\n> > myself don't understand the reasoning itself.\n> > \n> > Is the python code the only instance where we create a shared_ptr for\n> > the Camera? If that's true - then perhaps this patch should actually be\n> > squashed with the one that returns the Camera as a shared_ptr...\n> \n> This is not related to the patch 2.\n> \n> pybind11 needs a holder type that is used to manage the references to \n> the C++ instances. By default it's unique_ptr, but can be defined. We \n> use shared_ptr for many classes \n> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n> \n> I think normally it makes sense that the destructor must be public: if \n> pybind11 wrapper code can create the instances, it can also destruct \n> them. Here we don't create Cameras, nor destruct them, but the related \n> code is still created, causing the build error.\n> \n> I believe it should be possible to fix this in pybind11, by making the \n> template code to detect these kind of classes and skip the destructor \n> code. Maybe. Somehow. I have no idea how =).\n\nBut how could a shared_ptr skip calling the destructor? Isn't the\npurpose of a shared_ptr to keep the object 'alive' until the last user\nis removed? Therefore the shared_ptr has to be able to destruct the\nobject?\n\nIs there some magic that means the destructor can still be private for a\nshared_ptr in that instance?\n\nA quick google on shared_ptr private destructor brings up:\n\thttps://stackoverflow.com/a/8202667\n\nSaying it can be solved by making a custom deleter for the shared\npointer. Is that what we're missing perhaps?\n\n--\nKieran","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 50DC2BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6696660876;\n\tThu,  9 Dec 2021 11:34:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2AC460224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:34:54 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70630501;\n\tThu,  9 Dec 2021 11:34:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VrDF5XSX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639046094;\n\tbh=UwBL94r31+20ACcaKoJwmpyN9r9yphHumlcjfoTBzuE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VrDF5XSXEjBmNUYtou2FZP8v0K6CKhWCWygnEZL4gbaoJ0s9bV6IAeEJqmnlgDuc/\n\tuIgGXtzgD6aTXVWPO76s0Cii0wLIGsRlZYUwTpa05ATx7iTil6gfQRvkmNX9BCy/9n\n\tNQ6duJ0qTzOms310wBBW/SgdsdMjlWq/PrqNQZCM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>\n\t<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 Dec 2021 10:34:51 +0000","Message-ID":"<163904609199.2066819.9796248885725077869@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21704,"web_url":"https://patchwork.libcamera.org/comment/21704/","msgid":"<YbHgD0uE9+w+46xR@pendragon.ideasonboard.com>","date":"2021-12-09T10:53:03","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Dec 09, 2021 at 10:34:51AM +0000, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2021-12-09 10:22:16)\n> > On 09/12/2021 12:05, Kieran Bingham wrote:\n> > > Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n> > >> pybind11 needs a public destructor for Camera to be able to manage the\n> > >> shared_ptr. It's not clear why this is needed, and can it be fixed in\n> > >> pybind11.\n> > > \n> > > :-( The 'HACK' here seems to be the only thing that would count as a\n> > > blocker to integration.\n> > > \n> > > I wonder if this is really a HACK or if we just need to explain that the\n> > > destructor needs to be public to support the python bindings, but I\n> > > myself don't understand the reasoning itself.\n> > > \n> > > Is the python code the only instance where we create a shared_ptr for\n> > > the Camera? If that's true - then perhaps this patch should actually be\n> > > squashed with the one that returns the Camera as a shared_ptr...\n> > \n> > This is not related to the patch 2.\n> > \n> > pybind11 needs a holder type that is used to manage the references to \n> > the C++ instances. By default it's unique_ptr, but can be defined. We \n> > use shared_ptr for many classes \n> > (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n> > \n> > I think normally it makes sense that the destructor must be public: if \n> > pybind11 wrapper code can create the instances, it can also destruct \n> > them. Here we don't create Cameras, nor destruct them, but the related \n> > code is still created, causing the build error.\n> > \n> > I believe it should be possible to fix this in pybind11, by making the \n> > template code to detect these kind of classes and skip the destructor \n> > code. Maybe. Somehow. I have no idea how =).\n> \n> But how could a shared_ptr skip calling the destructor? Isn't the\n> purpose of a shared_ptr to keep the object 'alive' until the last user\n> is removed? Therefore the shared_ptr has to be able to destruct the\n> object?\n> \n> Is there some magic that means the destructor can still be private for a\n> shared_ptr in that instance?\n> \n> A quick google on shared_ptr private destructor brings up:\n> \thttps://stackoverflow.com/a/8202667\n> \n> Saying it can be solved by making a custom deleter for the shared\n> pointer. Is that what we're missing perhaps?\n\nWe already have a deleter, see Camera::create().","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 EF548BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:53:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5199160876;\n\tThu,  9 Dec 2021 11:53:36 +0100 (CET)","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 A74A160224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:53:34 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0EE1F501;\n\tThu,  9 Dec 2021 11:53:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ViWdymO3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639047214;\n\tbh=gShN216HszRgn2yQns/LHzKVSO+keGZXTGYI8vjyLZA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ViWdymO37jR8hg67cf1wlhIIBqQ2fL1SaphQ5kLtkR+fb9ej9kTlGuIw7QqgBAsD/\n\tB9/dJK62JSEOz+ETs13qPpcnPlNoQ3tO/KFeGNH05hv80gwd/olee9mb8JOUjErP3S\n\t4yAUqM+NpL5NiBpPqTk0vi1/7Laktsqbk/bZtUTg=","Date":"Thu, 9 Dec 2021 12:53:03 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YbHgD0uE9+w+46xR@pendragon.ideasonboard.com>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>\n\t<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>\n\t<163904609199.2066819.9796248885725077869@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163904609199.2066819.9796248885725077869@Monstersaurus>","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21705,"web_url":"https://patchwork.libcamera.org/comment/21705/","msgid":"<01092a19-4d35-7d94-912c-f9278fc010f5@ideasonboard.com>","date":"2021-12-09T10:54:07","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 09/12/2021 12:34, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2021-12-09 10:22:16)\n>> On 09/12/2021 12:05, Kieran Bingham wrote:\n>>> Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n>>>> pybind11 needs a public destructor for Camera to be able to manage the\n>>>> shared_ptr. It's not clear why this is needed, and can it be fixed in\n>>>> pybind11.\n>>>>\n>>>\n>>> :-( The 'HACK' here seems to be the only thing that would count as a\n>>> blocker to integration.\n>>>\n>>> I wonder if this is really a HACK or if we just need to explain that the\n>>> destructor needs to be public to support the python bindings, but I\n>>> myself don't understand the reasoning itself.\n>>>\n>>> Is the python code the only instance where we create a shared_ptr for\n>>> the Camera? If that's true - then perhaps this patch should actually be\n>>> squashed with the one that returns the Camera as a shared_ptr...\n>>\n>> This is not related to the patch 2.\n>>\n>> pybind11 needs a holder type that is used to manage the references to\n>> the C++ instances. By default it's unique_ptr, but can be defined. We\n>> use shared_ptr for many classes\n>> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n>>\n>> I think normally it makes sense that the destructor must be public: if\n>> pybind11 wrapper code can create the instances, it can also destruct\n>> them. Here we don't create Cameras, nor destruct them, but the related\n>> code is still created, causing the build error.\n>>\n>> I believe it should be possible to fix this in pybind11, by making the\n>> template code to detect these kind of classes and skip the destructor\n>> code. Maybe. Somehow. I have no idea how =).\n> \n> But how could a shared_ptr skip calling the destructor? Isn't the\n> purpose of a shared_ptr to keep the object 'alive' until the last user\n> is removed? Therefore the shared_ptr has to be able to destruct the\n> object?\n> \n> Is there some magic that means the destructor can still be private for a\n> shared_ptr in that instance?\n> \n> A quick google on shared_ptr private destructor brings up:\n> \thttps://stackoverflow.com/a/8202667\n> \n> Saying it can be solved by making a custom deleter for the shared\n> pointer. Is that what we're missing perhaps?\n\nThat is what is done in Camera.cpp, Camera::create(). Afaik that is the \nonly place where cameras are instantiated, and it, of course, has access \nto the destructor.\n\nThe pybind11 template code generates code to manage the wrapped class, \nand I think that code includes constructing and destructing of the \ninstance. Or maybe only destructing, as otherwise we should get a \nsimilar error for the constructor if that is required.\n\n  Tomi","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 16CF7BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 10:54:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C556A60822;\n\tThu,  9 Dec 2021 11:54:11 +0100 (CET)","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 BD04D60224\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 11:54:09 +0100 (CET)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6000A501;\n\tThu,  9 Dec 2021 11:54:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BrfzD/OE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639047249;\n\tbh=HnXZuo9eVZLjs7sWdzqPx12OOx3RgVScBEevXXS8Co0=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=BrfzD/OE/CXgV3UV+RjgrECtjY6dHZsohHYgyuoBXdUDIPyRyCNte8Be1y0/BLAeJ\n\tKkKaWFfMVsTh06vC0ylQ2fQreWLUxpO9pThO+ZAfIQfYsW286L/JTcO+suv2wqkV8M\n\tjf0e33FiJ5v/Q95cv8Kz7teYpE/BlxCGd9DbhPh4=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>\n\t<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>\n\t<163904609199.2066819.9796248885725077869@Monstersaurus>","From":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>","Message-ID":"<01092a19-4d35-7d94-912c-f9278fc010f5@ideasonboard.com>","Date":"Thu, 9 Dec 2021 12:54:07 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"<163904609199.2066819.9796248885725077869@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21716,"web_url":"https://patchwork.libcamera.org/comment/21716/","msgid":"<YbIy2r7p+lfR7/Tr@pendragon.ideasonboard.com>","date":"2021-12-09T16:46:18","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Dec 09, 2021 at 12:54:07PM +0200, Tomi Valkeinen wrote:\n> On 09/12/2021 12:34, Kieran Bingham wrote:\n> > Quoting Tomi Valkeinen (2021-12-09 10:22:16)\n> >> On 09/12/2021 12:05, Kieran Bingham wrote:\n> >>> Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n> >>>> pybind11 needs a public destructor for Camera to be able to manage the\n> >>>> shared_ptr. It's not clear why this is needed, and can it be fixed in\n> >>>> pybind11.\n> >>>>\n> >>>\n> >>> :-( The 'HACK' here seems to be the only thing that would count as a\n> >>> blocker to integration.\n> >>>\n> >>> I wonder if this is really a HACK or if we just need to explain that the\n> >>> destructor needs to be public to support the python bindings, but I\n> >>> myself don't understand the reasoning itself.\n> >>>\n> >>> Is the python code the only instance where we create a shared_ptr for\n> >>> the Camera? If that's true - then perhaps this patch should actually be\n> >>> squashed with the one that returns the Camera as a shared_ptr...\n> >>\n> >> This is not related to the patch 2.\n> >>\n> >> pybind11 needs a holder type that is used to manage the references to\n> >> the C++ instances. By default it's unique_ptr, but can be defined. We\n> >> use shared_ptr for many classes\n> >> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n> >>\n> >> I think normally it makes sense that the destructor must be public: if\n> >> pybind11 wrapper code can create the instances, it can also destruct\n> >> them. Here we don't create Cameras, nor destruct them, but the related\n> >> code is still created, causing the build error.\n> >>\n> >> I believe it should be possible to fix this in pybind11, by making the\n> >> template code to detect these kind of classes and skip the destructor\n> >> code. Maybe. Somehow. I have no idea how =).\n> > \n> > But how could a shared_ptr skip calling the destructor? Isn't the\n> > purpose of a shared_ptr to keep the object 'alive' until the last user\n> > is removed? Therefore the shared_ptr has to be able to destruct the\n> > object?\n> > \n> > Is there some magic that means the destructor can still be private for a\n> > shared_ptr in that instance?\n> > \n> > A quick google on shared_ptr private destructor brings up:\n> > \thttps://stackoverflow.com/a/8202667\n> > \n> > Saying it can be solved by making a custom deleter for the shared\n> > pointer. Is that what we're missing perhaps?\n> \n> That is what is done in Camera.cpp, Camera::create(). Afaik that is the \n> only place where cameras are instantiated, and it, of course, has access \n> to the destructor.\n> \n> The pybind11 template code generates code to manage the wrapped class, \n> and I think that code includes constructing and destructing of the \n> instance. Or maybe only destructing, as otherwise we should get a \n> similar error for the constructor if that is required.\n\nIf std::shared_ptr<> causes issue, we could manually implement a\nPyCamera (name to be bikeshedded, could be just Camera in a py::\nnamespace) C++ class that would wrap std::shared_ptr<Camera>.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 98860BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Dec 2021 16:46:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA0E56087A;\n\tThu,  9 Dec 2021 17:46:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F5BF607DE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Dec 2021 17:46:48 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 967AC501;\n\tThu,  9 Dec 2021 17:46:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pX/wWv3t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639068407;\n\tbh=PxD7xM1hbMdNDYDLH/tyPoJkK8c8sV1nu2SB2x9d0+Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pX/wWv3taL6UIvO6V6NfVlTDGn4lP4474YYuw40SdvLIVDBbowC2Iz+gdON47hytt\n\tOLRvJGSr2LCx5IB3ZG5EvTPPFQNQXsUY7tWGOIWFic+k5MSB6AgGbwjkJfnsQmMY2+\n\toAxMz2k7CGtei3ULo7aArRdmaqkm3q0v9YYyXdY8=","Date":"Thu, 9 Dec 2021 18:46:18 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>","Message-ID":"<YbIy2r7p+lfR7/Tr@pendragon.ideasonboard.com>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>\n\t<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>\n\t<163904609199.2066819.9796248885725077869@Monstersaurus>\n\t<01092a19-4d35-7d94-912c-f9278fc010f5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<01092a19-4d35-7d94-912c-f9278fc010f5@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21801,"web_url":"https://patchwork.libcamera.org/comment/21801/","msgid":"<55ce41ae-5034-8ace-dabf-6bd2d278639d@ideasonboard.com>","date":"2021-12-15T14:33:15","subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 09/12/2021 18:46, Laurent Pinchart wrote:\n> On Thu, Dec 09, 2021 at 12:54:07PM +0200, Tomi Valkeinen wrote:\n>> On 09/12/2021 12:34, Kieran Bingham wrote:\n>>> Quoting Tomi Valkeinen (2021-12-09 10:22:16)\n>>>> On 09/12/2021 12:05, Kieran Bingham wrote:\n>>>>> Quoting Tomi Valkeinen (2021-12-09 09:29:02)\n>>>>>> pybind11 needs a public destructor for Camera to be able to manage the\n>>>>>> shared_ptr. It's not clear why this is needed, and can it be fixed in\n>>>>>> pybind11.\n>>>>>>\n>>>>>\n>>>>> :-( The 'HACK' here seems to be the only thing that would count as a\n>>>>> blocker to integration.\n>>>>>\n>>>>> I wonder if this is really a HACK or if we just need to explain that the\n>>>>> destructor needs to be public to support the python bindings, but I\n>>>>> myself don't understand the reasoning itself.\n>>>>>\n>>>>> Is the python code the only instance where we create a shared_ptr for\n>>>>> the Camera? If that's true - then perhaps this patch should actually be\n>>>>> squashed with the one that returns the Camera as a shared_ptr...\n>>>>\n>>>> This is not related to the patch 2.\n>>>>\n>>>> pybind11 needs a holder type that is used to manage the references to\n>>>> the C++ instances. By default it's unique_ptr, but can be defined. We\n>>>> use shared_ptr for many classes\n>>>> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).\n>>>>\n>>>> I think normally it makes sense that the destructor must be public: if\n>>>> pybind11 wrapper code can create the instances, it can also destruct\n>>>> them. Here we don't create Cameras, nor destruct them, but the related\n>>>> code is still created, causing the build error.\n>>>>\n>>>> I believe it should be possible to fix this in pybind11, by making the\n>>>> template code to detect these kind of classes and skip the destructor\n>>>> code. Maybe. Somehow. I have no idea how =).\n>>>\n>>> But how could a shared_ptr skip calling the destructor? Isn't the\n>>> purpose of a shared_ptr to keep the object 'alive' until the last user\n>>> is removed? Therefore the shared_ptr has to be able to destruct the\n>>> object?\n>>>\n>>> Is there some magic that means the destructor can still be private for a\n>>> shared_ptr in that instance?\n>>>\n>>> A quick google on shared_ptr private destructor brings up:\n>>> \thttps://stackoverflow.com/a/8202667\n>>>\n>>> Saying it can be solved by making a custom deleter for the shared\n>>> pointer. Is that what we're missing perhaps?\n>>\n>> That is what is done in Camera.cpp, Camera::create(). Afaik that is the\n>> only place where cameras are instantiated, and it, of course, has access\n>> to the destructor.\n>>\n>> The pybind11 template code generates code to manage the wrapped class,\n>> and I think that code includes constructing and destructing of the\n>> instance. Or maybe only destructing, as otherwise we should get a\n>> similar error for the constructor if that is required.\n> \n> If std::shared_ptr<> causes issue, we could manually implement a\n> PyCamera (name to be bikeshedded, could be just Camera in a py::\n> namespace) C++ class that would wrap std::shared_ptr<Camera>.\n\nLooks like there's a proposed fix:\n\nhttps://github.com/jagerman/pybind11/commit/53be81931f35313f70affc9826bdfed9820cce2c\n\nbut the pull-req hasn't been approved:\n\nhttps://github.com/pybind/pybind11/pull/2067\n\nThere also seems to be a fork (?) or pybind11 which implements something related,\nwhich would possibly help here (or not):\n\nhttps://github.com/pybind/pybind11/blob/49f8f60ec4254e0f55db3c095c945210bcb43ad2/README_smart_holder.rst\n\nI'm a bit reluctant going to the wrapper class just to avoid this issue,\nas I fear it will create a bit of a mess in the code.\n\n  Tomi","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 89A32BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 15 Dec 2021 14:33:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9611560894;\n\tWed, 15 Dec 2021 15:33:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CD2760113\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 15 Dec 2021 15:33:19 +0100 (CET)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8192292;\n\tWed, 15 Dec 2021 15:33:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XdX538/K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639578799;\n\tbh=LU1kuYjw7f7Suk2PV50N1hwAzbb0GJLSkhWeFavgmus=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XdX538/KdH5VFFF21jcmQCQAZV3/Sib0WXw6v3WewXiWUwEZO6yV1AN2t1zLAvduT\n\tTMIgmdIxomTPLBlgTeK/hBixoQzp/YY2AAYWryBDdJMo/JM11bUT1e3ZEvBPh1vmv4\n\tuUpBiEK13P0CmKJ7E0WYNZvTbRoOgCHi27RgpTNg=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20211209092906.37303-1-tomi.valkeinen@ideasonboard.com>\n\t<20211209092906.37303-2-tomi.valkeinen@ideasonboard.com>\n\t<163904435684.2066819.9235024135570227528@Monstersaurus>\n\t<aa417d80-b273-e5a3-a262-925413ae68eb@ideasonboard.com>\n\t<163904609199.2066819.9796248885725077869@Monstersaurus>\n\t<01092a19-4d35-7d94-912c-f9278fc010f5@ideasonboard.com>\n\t<YbIy2r7p+lfR7/Tr@pendragon.ideasonboard.com>","From":"Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>","Message-ID":"<55ce41ae-5034-8ace-dabf-6bd2d278639d@ideasonboard.com>","Date":"Wed, 15 Dec 2021 16:33:15 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"<YbIy2r7p+lfR7/Tr@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]