[{"id":13959,"web_url":"https://patchwork.libcamera.org/comment/13959/","msgid":"<20201129232708.GB5893@pendragon.ideasonboard.com>","date":"2020-11-29T23:27:08","subject":"Re: [libcamera-devel] [RFC v2 1/4] 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 Tomi,\n\nThank you for the patch.\n\nOn Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote:\n> Not clear why we need this with pybind11...\n\nWe'll have to figure it out :-)\n\nI've been considering a while ago handling camera instances a bit\ndifferently, with a facade object create by libcamera (and returned a\nunique pointer), with a reference-counted (through shared_ptr) backend\nobject not visible to applications. We could implement this, either in\nthe libcamera core, or in the python bindings, if it helps.\n\n> Signed-off-by: Tomi Valkeinen \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 5c5f1a05..d8e0bdbc 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -106,10 +106,10 @@ public:\n> \tint start();\n> \tint stop();\n> \n> +\t~Camera();\n> private:\n> \tCamera(PipelineHandler *pipe, const std::string &id,\n> \t const std::set &streams);\n> -\t~Camera();\n> \n> \tfriend class PipelineHandler;\n> \tvoid disconnect();","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 48444BE177\n\tfor ;\n\tSun, 29 Nov 2020 23:27:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A444B6347D;\n\tMon, 30 Nov 2020 00:27:18 +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 0B9416346E\n\tfor ;\n\tMon, 30 Nov 2020 00:27:17 +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 76F7C97E;\n\tMon, 30 Nov 2020 00:27:16 +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=\"WhHHUyzM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606692436;\n\tbh=Ak1lwB6tPAQgPD7n02crVSwZ0qCqyQeK2Bf/zxU/SW0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WhHHUyzMVF3DwbRRsJzptT4wJaApxQ2SHJlHxeDpJF5Lmh14TdBLoj+wqm2/VUt+s\n\tWQ+fNDiH/VavFP57p73GE/vNYXN9dBW6c5K0AS1BGRgAw+Lh8MOo5BOd7XqRQDy/v3\n\tO9nkUK7Lln3iNhk+cpOnuT1s/q4hxPRIQUSVuwmM=","Date":"Mon, 30 Nov 2020 01:27:08 +0200","From":"Laurent Pinchart ","To":"Tomi Valkeinen ","Message-ID":"<20201129232708.GB5893@pendragon.ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-2-tomi.valkeinen@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127133738.880859-2-tomi.valkeinen@iki.fi>","Subject":"Re: [libcamera-devel] [RFC v2 1/4] hack: Camera public destructor","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13980,"web_url":"https://patchwork.libcamera.org/comment/13980/","msgid":"","date":"2020-11-30T14:26:48","subject":"Re: [libcamera-devel] [RFC v2 1/4] hack: Camera public destructor","submitter":{"id":70,"url":"https://patchwork.libcamera.org/api/people/70/","name":"Tomi Valkeinen","email":"tomi.valkeinen@iki.fi"},"content":"On 30/11/2020 01:27, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote:\n>> Not clear why we need this with pybind11...\n> \n> We'll have to figure it out :-)\n\nMy guess is that pybind11 generates code to create a shared_ptr. You need a public\ndestructor for that.\n\nAh, no, Camera constructor is private too, so that cannot be the case.\n\n> I've been considering a while ago handling camera instances a bit\n> differently, with a facade object create by libcamera (and returned a\n> unique pointer), with a reference-counted (through shared_ptr) backend\n> object not visible to applications. We could implement this, either in\n> the libcamera core, or in the python bindings, if it helps.\n\nWhat would be the benefit of that approach in libcamera core? Isn't that just re-implementing the\nsame feature with more complex code? Unless you want to hide something with the backend object that\ncan't be hidden at the moment.\n\nI'm not sure, but we might have the same problem with unique_ptr too. If something in pybind11\nrequires the destructor to be visible with shared_ptr, that could also be the case with unique_ptr.\n\nI have considered adding facades/wrappers for some classes in the py bindings, so that I could store\nextra state in those objects. But it does get complex, as I need to re-use existing objects instead\nof just creating a fresh one every time I get an instance from c++ side.\n\nThat's a bit different than what you're suggesting, but would also remove the use of\nshared_ptr from the bindings itself.\n\nBut if all this is just to avoid making the Camera destructor public, I have to ask if it's worth it =).\n\n Tomi","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 EC492BE176\n\tfor ;\n\tMon, 30 Nov 2020 14:26:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 760F86349F;\n\tMon, 30 Nov 2020 15:26:52 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D55EF63320\n\tfor ;\n\tMon, 30 Nov 2020 15:26:50 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id q8so9183311ljc.12\n\tfor ;\n\tMon, 30 Nov 2020 06:26:50 -0800 (PST)","from [192.168.1.111] (91-152-83-50.elisa-laajakaista.fi.\n\t[91.152.83.50]) by smtp.gmail.com with ESMTPSA id\n\tw21sm2516324lff.280.2020.11.30.06.26.49\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 30 Nov 2020 06:26:49 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"e9zv6pEn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=sender:subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=adBt5PNo8gCQB8XSPOwD2HKkJKfo/14swrewcXuZDBI=;\n\tb=e9zv6pEnZBe4t5yhSvz+/+tlONW73wZzhze+MGL9/lxewfQPIzUsouVEXC5+0Wegla\n\txwvrORGcxhwFMdLTizpULInZ2Vqqeg6puc3lHPTaSoG5/UM8/e/dFPkuuuN6BWmaZY7p\n\tBIwweiJlfW/mmO52UCm2cUdh+y0gwmYSAJGEjeSnVRh4HXj2rCGX0EczoYSF/VDw4QNx\n\tyyzHMJm+d3fDZ71q3yfYGvsYaFcSzqN0n2EEPwCzaNvqzcaunePcChsc2fzOCRiDvTGf\n\t1ab+1MncqCwRJhGXUEyj2D4SBKGmUBnWnzbwIHgyle+zwiCapAAmaJCRPrZEw602WqZy\n\tD2VA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:sender:subject:to:cc:references:from:message-id\n\t:date:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=adBt5PNo8gCQB8XSPOwD2HKkJKfo/14swrewcXuZDBI=;\n\tb=XN/b/N5xX6gc91mhFCrQ02XWvFkZRRkYqkWEfkWDwYRxUu8PxPwsKflVhVyjgC7iCm\n\tI+5rhKiQLhsldRuuXizCta+fIRLDgB8O+DdUL/LLHmzIsCPF50d5X1EV7JBwljJ4FQMt\n\ty8lh3vg4PFBmWRCg9ouG21gLmKsYkqdI/oafng2w406s09NUhTfOi1BLXDqNAH0dVlUK\n\ta+NQ0kMQaHjRAZv8m+5sQNW7VUyIoCHtWPjIOPQVDAZeDAToAgdDShIjtRNdAS/c9yBU\n\tJwjbozAikfA5RdIlDeIxknjGp0Xqv8Y/2Du29LuDFDPGEB3N2ShW5hcy9GrbJl1sYDkK\n\tWY5w==","X-Gm-Message-State":"AOAM5329W+HG8CAZvc/lTtdbydDmk1ECLBWifAfH0FDCTt/692/no/gF\n\tgHj3e5+/dG5uEaj1Qm/JGk7HxOLRCNs=","X-Google-Smtp-Source":"ABdhPJwqIMEOXv8MrX2/LBJHFIX4w/MuE5aOCw9AQcoKmE8G2iHQXHQLgkrzRzwtiZMQFc20en3wog==","X-Received":"by 2002:a2e:580d:: with SMTP id\n\tm13mr10345408ljb.141.1606746409890; \n\tMon, 30 Nov 2020 06:26:49 -0800 (PST)","To":"Laurent Pinchart ","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-2-tomi.valkeinen@iki.fi>\n\t<20201129232708.GB5893@pendragon.ideasonboard.com>","From":"Tomi Valkeinen ","Message-ID":"","Date":"Mon, 30 Nov 2020 16:26:48 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201129232708.GB5893@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC v2 1/4] hack: Camera public destructor","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13982,"web_url":"https://patchwork.libcamera.org/comment/13982/","msgid":"<20201130155314.GA14465@pendragon.ideasonboard.com>","date":"2020-11-30T15:53:14","subject":"Re: [libcamera-devel] [RFC v2 1/4] 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 Tomi,\n\nOn Mon, Nov 30, 2020 at 04:26:48PM +0200, Tomi Valkeinen wrote:\n> On 30/11/2020 01:27, Laurent Pinchart wrote:\n> > Hi Tomi,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote:\n> >> Not clear why we need this with pybind11...\n> > \n> > We'll have to figure it out :-)\n> \n> My guess is that pybind11 generates code to create a shared_ptr. You need a public\n> destructor for that.\n> \n> Ah, no, Camera constructor is private too, so that cannot be the case.\n\nIn pybind11::class_::init_holder(), \n\n 1. /// Initialize holder object, variant 1: object derives from enable_shared_from_this\n 2. template \n 3. static void init_holder(detail::instance *inst, detail::value_and_holder &v_h,\n 4. const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) {\n 5. try {\n 6. auto sh = std::dynamic_pointer_cast(\n 7. v_h.value_ptr()->shared_from_this());\n 8. if (sh) {\n 9. new (std::addressof(v_h.holder())) holder_type(std::move(sh));\n10. v_h.set_holder_constructed();\n11. }\n12. } catch (const std::bad_weak_ptr &) {}\n13.\n14. if (!v_h.holder_constructed() && inst->owned) {\n15. new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr());\n16. v_h.set_holder_constructed();\n17. }\n18. }\n\nOn line 15 a holder_type is constructed. holder_type is\nstd::shared_ptr, given that the bindings use\n\n\tpy::class_>(m, \"Camera\", py::dynamic_attr())\n\nI think this code will never be executed in practice, as the holder\nshould always be initialized from a std::shared_ptr<>, but the compiler\ncan't know that.\n\n> > I've been considering a while ago handling camera instances a bit\n> > differently, with a facade object create by libcamera (and returned a\n> > unique pointer), with a reference-counted (through shared_ptr) backend\n> > object not visible to applications. We could implement this, either in\n> > the libcamera core, or in the python bindings, if it helps.\n> \n> What would be the benefit of that approach in libcamera core? Isn't that just re-implementing the\n> same feature with more complex code? Unless you want to hide something with the backend object that\n> can't be hidden at the moment.\n\nIt was partly about hiding the Camera object private fields, but that\nhas since been done through usage of the d-pointer design pattern. The\nother reason is to avoid proliferation of std::shared_ptr<> on the\napplication side. It could help with other language bindings too.\n\n> I'm not sure, but we might have the same problem with unique_ptr too. If something in pybind11\n> requires the destructor to be visible with shared_ptr, that could also be the case with unique_ptr.\n\nThe destructor for the facade object would be visible. That's the whole\npoint, it would be constructed by libcamera, but destroyed by\napplications. I'm however not sure how practical that would be, given\nthat we wouldn't be able to expose a vector of cameras anymore (at least\nnot in the same way).\n\n> I have considered adding facades/wrappers for some classes in the py bindings, so that I could store\n> extra state in those objects. But it does get complex, as I need to re-use existing objects instead\n> of just creating a fresh one every time I get an instance from c++ side.\n\nI have a feeling we'll have to do so at some point, for some of the\nobjects, but have no proof for now :-)\n\n> That's a bit different than what you're suggesting, but would also remove the use of\n> shared_ptr from the bindings itself.\n> \n> But if all this is just to avoid making the Camera destructor public, I have to ask if it's worth it =).","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 68670BE176\n\tfor ;\n\tMon, 30 Nov 2020 15:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4D6C63320;\n\tMon, 30 Nov 2020 16:53:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D0DF63320\n\tfor ;\n\tMon, 30 Nov 2020 16:53:23 +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 F2F0697E;\n\tMon, 30 Nov 2020 16:53:22 +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=\"EBNieW77\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606751603;\n\tbh=fkjxXtGv9yZC6Lkv0Dpm2Nn/eGo+Aw/ezrGiE2w6wh4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EBNieW77ZzdvdmfsOcjUf+begh2qES7Xgqyyxakmc2srGYTYS01lPy1N1pmucExI9\n\tTZKXXbZTOFEvn380NFfrsxep3JwtuyFNfHf3pDskeOu1n6r8nph9EAj/rixUqt+AmW\n\tIK6vZ55JFFi8bL5bpTSN/M0fzqXWKOodVGxRL54k=","Date":"Mon, 30 Nov 2020 17:53:14 +0200","From":"Laurent Pinchart ","To":"Tomi Valkeinen ","Message-ID":"<20201130155314.GA14465@pendragon.ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-2-tomi.valkeinen@iki.fi>\n\t<20201129232708.GB5893@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC v2 1/4] hack: Camera public destructor","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]