[{"id":13958,"web_url":"https://patchwork.libcamera.org/comment/13958/","msgid":"<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>","date":"2020-11-29T21:08:28","subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Tomi,\n\nOn 27/11/2020 13:37, Tomi Valkeinen wrote:\n> Py bindings need to find out which camera is the source of the completed\n> Request. Request already has a private field for the Camera, so we can\n> just expose it via a getter.\n\nInterestingly, some time ago I posted a similar (but simpler?) patch for\nthis.\n\n[PATCH] libcamera: request: Facilitate retrieval of the camera\n\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html\n\nWhich was partially rejected by Niklas, however he did give me an RB tag.\n\nI sort of thought this was beneficial as it helps get directly to the\n'correct' Camera in any callbacks.\n\nInteresting that you've used a shared_ptr rather than where I directly\npass the pointer, and I think that's likely more correct as the expected\nuse might be to take the event in a callback, and immediately pass it on\nto another thread - so a shared pointer makes a bit more sense then to\nkeep things safe.\n\n\n\n> Signed-off-by: Tomi Valkeinen \n> ---\n> include/libcamera/request.h | 1 +\n> src/libcamera/request.cpp | 5 +++++\n> 2 files changed, 6 insertions(+)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 655b1324..f98ef767 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -56,6 +56,7 @@ public:\n> \n> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> \n> + std::shared_ptr camera() const;\n> private:\n> \tfriend class PipelineHandler;\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index a68684ef..5a50ec6b 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -216,6 +216,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> \treturn it->second;\n> }\n> \n\n\nPerhaps just add this ;-) (stolen/adapted from my patch)\n\n+/**\n+ * \\fn Request::camera()\n+ * \\brief Retrieve the camera that owns the request\n+ *\n+ * \\return A shared pointer to the camera associated with the request\n+ */\n\nWith a version with that, I'd give this a:\n\nReviewed-by: Kieran Bingham \n\nBut given previous push-back, I'll hope for further acceptance from the\nothers before we consider dropping the 'HACK:' prefix ;-)\n\n--\nKieran\n\n\n> +std::shared_ptr Request::camera() const\n> +{\n> +\treturn camera_->shared_from_this();\n> +}\n> +\n> /**\n> * \\fn Request::metadata()\n> * \\brief Retrieve the request's metadata\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 75783BE176\n\tfor ;\n\tSun, 29 Nov 2020 21:08:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC9076347C;\n\tSun, 29 Nov 2020 22:08:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0460B63400\n\tfor ;\n\tSun, 29 Nov 2020 22:08:30 +0100 (CET)","from [192.168.0.20]\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 6A2439A8;\n\tSun, 29 Nov 2020 22:08:30 +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=\"FoFROuXv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606684110;\n\tbh=oLzXLGzKzOrOIZQPjcpabZLmyvUvOkVAVjreRBOnoTI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FoFROuXvv+KXsQ2E+jCNf4+4NmBNrSyLyvSk0VhV+C5zodpoy0ocvv0i0qfCPAE9l\n\tC0eIFXYZE1Uh7twFTiGwrgsBElhbuym7H0QOMsscfr0BQl7m8qkHAc/zCjibgcr+3+\n\tyOdIJPY6TdttgS3X1GQjQfx7HGGhSAmikIVvc1tE=","To":"Tomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-3-tomi.valkeinen@iki.fi>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>","Date":"Sun, 29 Nov 2020 21:08:28 +0000","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":"<20201127133738.880859-3-tomi.valkeinen@iki.fi>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"Tomi Valkeinen ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13960,"web_url":"https://patchwork.libcamera.org/comment/13960/","msgid":"<20201129234252.GC5893@pendragon.ideasonboard.com>","date":"2020-11-29T23:42:52","subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran and Tomi,\n\nOn Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote:\n> On 27/11/2020 13:37, Tomi Valkeinen wrote:\n> > Py bindings need to find out which camera is the source of the completed\n> > Request. Request already has a private field for the Camera, so we can\n> > just expose it via a getter.\n\nNote that the camera_ member is actually not used. We should either use\nit, or remove it :-)\n\n> Interestingly, some time ago I posted a similar (but simpler?) patch for\n> this.\n> \n> [PATCH] libcamera: request: Facilitate retrieval of the camera\n> \n> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html\n> \n> Which was partially rejected by Niklas, however he did give me an RB tag.\n> \n> I sort of thought this was beneficial as it helps get directly to the\n> 'correct' Camera in any callbacks.\n> \n> Interesting that you've used a shared_ptr rather than where I directly\n> pass the pointer, and I think that's likely more correct as the expected\n> use might be to take the event in a callback, and immediately pass it on\n> to another thread - so a shared pointer makes a bit more sense then to\n> keep things safe.\n\nI'm not entirely sure about that. shared_ptr<> has a tendency to spread,\nand sometimes borrowed references are enough. In this particular case,\nrequests should all complete before the application receives the camera\ndisconnection signal. Borrowed references to the camera in the\ncompletion handler should thus be valid. Request processing is then\ntypically deferred to the main thread, but for cancelled requests, a\ndifferent code path can be used.\n\nI'll review the patch that makes use of this to see what's best.\n\n> > Signed-off-by: Tomi Valkeinen \n> > ---\n> > include/libcamera/request.h | 1 +\n> > src/libcamera/request.cpp | 5 +++++\n> > 2 files changed, 6 insertions(+)\n> > \n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 655b1324..f98ef767 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -56,6 +56,7 @@ public:\n> > \n> > \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> > \n> > + std::shared_ptr camera() const;\n\nDoesn't checkstyle.py warn about spaces uses for indentation ? There's\nalso a missing blank line.\n\n> > private:\n> > \tfriend class PipelineHandler;\n> > \n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index a68684ef..5a50ec6b 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -216,6 +216,11 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > \treturn it->second;\n> > }\n> > \n> \n> Perhaps just add this ;-) (stolen/adapted from my patch)\n> \n> +/**\n> + * \\fn Request::camera()\n> + * \\brief Retrieve the camera that owns the request\n> + *\n> + * \\return A shared pointer to the camera associated with the request\n> + */\n> \n> With a version with that, I'd give this a:\n> \n> Reviewed-by: Kieran Bingham \n> \n> But given previous push-back, I'll hope for further acceptance from the\n> others before we consider dropping the 'HACK:' prefix ;-)\n> \n> > +std::shared_ptr Request::camera() const\n> > +{\n> > +\treturn camera_->shared_from_this();\n> > +}\n> > +\n> > /**\n> > * \\fn Request::metadata()\n> > * \\brief Retrieve the request's metadata","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 1FEC6BE176\n\tfor ;\n\tSun, 29 Nov 2020 23:43:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8117A6347D;\n\tMon, 30 Nov 2020 00:43:03 +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 9E2B36346E\n\tfor ;\n\tMon, 30 Nov 2020 00:43:01 +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 0A37C97E;\n\tMon, 30 Nov 2020 00:43:00 +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=\"DkhP8Vr4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606693381;\n\tbh=03Erc+HGYU888LcJvci1ombfv2ipDxurIVuYM6CIfXA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DkhP8Vr4lfQjVpSeLPYXTGND9smQ42AsHEIhau7znNxfuKTIcdolzzdU6j5HTxSR1\n\thnv/uqaL1hUM9vOmKOzuYOpt9doLpfsJUpAv89wtH5LpuE4/Q+D9TNXb2wYoZxYjEB\n\tNl5rZpoRSE4hDYFKyXM2ULjPhMsU0eishtsuSw64=","Date":"Mon, 30 Nov 2020 01:42:52 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20201129234252.GC5893@pendragon.ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-3-tomi.valkeinen@iki.fi>\n\t<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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":"Tomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org, \n\tTomi Valkeinen ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13978,"web_url":"https://patchwork.libcamera.org/comment/13978/","msgid":"<0befc39e-249f-8f1c-4385-8080b7cbca1b@iki.fi>","date":"2020-11-30T12:48:11","subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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:42, Laurent Pinchart wrote:\n> Hi Kieran and Tomi,\n> \n> On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote:\n>> On 27/11/2020 13:37, Tomi Valkeinen wrote:\n>>> Py bindings need to find out which camera is the source of the completed\n>>> Request. Request already has a private field for the Camera, so we can\n>>> just expose it via a getter.\n> \n> Note that the camera_ member is actually not used. We should either use\n> it, or remove it :-)\n> \n>> Interestingly, some time ago I posted a similar (but simpler?) patch for\n>> this.\n>>\n>> [PATCH] libcamera: request: Facilitate retrieval of the camera\n>>\n>> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html\n>>\n>> Which was partially rejected by Niklas, however he did give me an RB tag.\n>>\n>> I sort of thought this was beneficial as it helps get directly to the\n>> 'correct' Camera in any callbacks.\n>>\n>> Interesting that you've used a shared_ptr rather than where I directly\n>> pass the pointer, and I think that's likely more correct as the expected\n>> use might be to take the event in a callback, and immediately pass it on\n>> to another thread - so a shared pointer makes a bit more sense then to\n>> keep things safe.\n> \n> I'm not entirely sure about that. shared_ptr<> has a tendency to spread,\n> and sometimes borrowed references are enough. In this particular case,\n> requests should all complete before the application receives the camera\n> disconnection signal. Borrowed references to the camera in the\n> completion handler should thus be valid. Request processing is then\n> typically deferred to the main thread, but for cancelled requests, a\n> different code path can be used.\n\nWe can return Camera* here, and I can convert it to a shared_ptr in the binding code.\n\nI don't know what you mean with \"has tendency to spread\". The camera is allocated as shared_ptr, so\ngenerally speaking, it should always be shared_ptr.\n\nIn some cases we could pass it as Camera* as an optimization, e.g. for a function that we know only\ndoes a thing X, and does not store the Camera pointer anywhere. Storing a Camera* is not correct in\nmy opinion, although it can of course work if the lifetime or that pointer is well defined and\nenforced. But it's pretty easy to get that wrong, and it should be considered if such optimization\nis worth it (maybe it is here with Requests).\n\nBut when talking about the python bindings, a Camera* is not ok, we can't pass such a thing to\npython. Either python owns the instance (unique_ptr, which we can create from a pointer) or it\ndoesn't (shared_ptr). Well, we can also build more elaborate ownership containers, but the rules\nmust be clear and defined in the container code.\n\nIn this case, as Camera is a shared_ptr, and we store the Camera to a list which is processed later,\nI think it's clear that we should use shared_ptr (either as I do in this patch, or convert Camera*\nto shared_ptr in the binding code).\n\n> I'll review the patch that makes use of this to see what's best.\n> \n>>> Signed-off-by: Tomi Valkeinen \n>>> ---\n>>> include/libcamera/request.h | 1 +\n>>> src/libcamera/request.cpp | 5 +++++\n>>> 2 files changed, 6 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>>> index 655b1324..f98ef767 100644\n>>> --- a/include/libcamera/request.h\n>>> +++ b/include/libcamera/request.h\n>>> @@ -56,6 +56,7 @@ public:\n>>> \n>>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n>>> \n>>> + std::shared_ptr camera() const;\n> \n> Doesn't checkstyle.py warn about spaces uses for indentation ? There's\n> also a missing blank line.\n\nIt does, I just didn't run it... Not sure how I managed to indent with spaces, though.\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 424C5BE176\n\tfor ;\n\tMon, 30 Nov 2020 12:48:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDA0A634A1;\n\tMon, 30 Nov 2020 13:48:14 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9D7563320\n\tfor ;\n\tMon, 30 Nov 2020 13:48:13 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id f18so17686387ljg.9\n\tfor ;\n\tMon, 30 Nov 2020 04:48:13 -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\tv11sm2451831lfb.185.2020.11.30.04.48.12\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 30 Nov 2020 04:48:12 -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=\"Y5aSTEkE\"; 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=bxv8luznxFeWTnEm24JKWzcd/jsF7oxIcpBCloTpUXA=;\n\tb=Y5aSTEkEYT7y8SCjsFngXuKb1UsxpQxpi0J1Ul2g39Lhd0Zn3TePF2ww4Xt1MwEnnV\n\tGeZTs6smLFVstCGzLNtwGg5OUlOOIYU6lkgNYcFwaqPu5H8X/eRXiurcenjPJtFcVpOo\n\tD2/+Z6ll1hOtn9+rFikEgjAcfaKZPAvjS9N6nmGOByBmQYoFXwHEOHP96ENxzI2Wxert\n\txYuhVJqEnRJ0kZUdPcACZoewQwg4x8oD4mGNImqxidEzNtvNfdPEPWehhgIGzSnhxV2z\n\twAP4Er54MHF1pVnS2otWN6yMgSoq7LYqYyvhJgrnPRQWAXkvDr3q+WuFyp32Q6d9AcPP\n\tYdjg==","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=bxv8luznxFeWTnEm24JKWzcd/jsF7oxIcpBCloTpUXA=;\n\tb=DLMnmU2YXcHYmluskdjo/2hD0fSN+lkwpI4M32LqdpCBD4LYf6BdM9DOFMka6SFp3J\n\tp85lbxw85n/cH8x2ZaFhWFrmu9LOO4dPw+6r9KkB2/B3tTorcJvZbw/ZATGEA/9jUchc\n\toyYpvNM6TZAaOO/aIbLa5n1NuVpDxdkwe6LRnst0NgoRz4HmavMuKgz0AOpqJlqY3e1z\n\tHQWyvINyUznaEFKyU7jh9Cf5nlbMSfHv8JpU9wEmQWauPMLdLuThpqMwxwxMZ2BqW298\n\t4VwGRZ6tJZ8/s/nGXFNPXgn5eYMNdksS3Q6UWSnzg3vtJUs3NKqJs7bS1IsQEkwCEMxi\n\to0gw==","X-Gm-Message-State":"AOAM530WMusT/m2ftRORllwAc/xowPL+4IrtKGGGOxBHvjr0bkhMsTtt\n\txtVIMRHMd8Qa38FTXovUa3dRA0A5Kbc=","X-Google-Smtp-Source":"ABdhPJzSyCM0vQfMVgE/B2HLJAUvPzVFYm7UKgslf7C3o86vZ5uzKhV6WgfmKk6kW2qblt49Wszijg==","X-Received":"by 2002:a2e:884f:: with SMTP id\n\tz15mr9409787ljj.200.1606740492877; \n\tMon, 30 Nov 2020 04:48:12 -0800 (PST)","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-3-tomi.valkeinen@iki.fi>\n\t<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>\n\t<20201129234252.GC5893@pendragon.ideasonboard.com>","From":"Tomi Valkeinen ","Message-ID":"<0befc39e-249f-8f1c-4385-8080b7cbca1b@iki.fi>","Date":"Mon, 30 Nov 2020 14:48:11 +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":"<20201129234252.GC5893@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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":"Tomi Valkeinen ,\n\tlibcamera-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":13983,"web_url":"https://patchwork.libcamera.org/comment/13983/","msgid":"<20201130160021.GB14465@pendragon.ideasonboard.com>","date":"2020-11-30T16:00:21","subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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 02:48:11PM +0200, Tomi Valkeinen wrote:\n> On 30/11/2020 01:42, Laurent Pinchart wrote:\n> > On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote:\n> >> On 27/11/2020 13:37, Tomi Valkeinen wrote:\n> >>> Py bindings need to find out which camera is the source of the completed\n> >>> Request. Request already has a private field for the Camera, so we can\n> >>> just expose it via a getter.\n> > \n> > Note that the camera_ member is actually not used. We should either use\n> > it, or remove it :-)\n> > \n> >> Interestingly, some time ago I posted a similar (but simpler?) patch for\n> >> this.\n> >>\n> >> [PATCH] libcamera: request: Facilitate retrieval of the camera\n> >>\n> >> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html\n> >>\n> >> Which was partially rejected by Niklas, however he did give me an RB tag.\n> >>\n> >> I sort of thought this was beneficial as it helps get directly to the\n> >> 'correct' Camera in any callbacks.\n> >>\n> >> Interesting that you've used a shared_ptr rather than where I directly\n> >> pass the pointer, and I think that's likely more correct as the expected\n> >> use might be to take the event in a callback, and immediately pass it on\n> >> to another thread - so a shared pointer makes a bit more sense then to\n> >> keep things safe.\n> > \n> > I'm not entirely sure about that. shared_ptr<> has a tendency to spread,\n> > and sometimes borrowed references are enough. In this particular case,\n> > requests should all complete before the application receives the camera\n> > disconnection signal. Borrowed references to the camera in the\n> > completion handler should thus be valid. Request processing is then\n> > typically deferred to the main thread, but for cancelled requests, a\n> > different code path can be used.\n> \n> We can return Camera* here, and I can convert it to a shared_ptr in the binding code.\n> \n> I don't know what you mean with \"has tendency to spread\". The camera is allocated as shared_ptr, so\n> generally speaking, it should always be shared_ptr.\n> \n> In some cases we could pass it as Camera* as an optimization, e.g. for a function that we know only\n> does a thing X, and does not store the Camera pointer anywhere. Storing a Camera* is not correct in\n> my opinion, although it can of course work if the lifetime or that pointer is well defined and\n> enforced. But it's pretty easy to get that wrong, and it should be considered if such optimization\n> is worth it (maybe it is here with Requests).\n> \n> But when talking about the python bindings, a Camera* is not ok, we can't pass such a thing to\n> python. Either python owns the instance (unique_ptr, which we can create from a pointer) or it\n> doesn't (shared_ptr). Well, we can also build more elaborate ownership containers, but the rules\n> must be clear and defined in the container code.\n> \n> In this case, as Camera is a shared_ptr, and we store the Camera to a list which is processed later,\n> I think it's clear that we should use shared_ptr (either as I do in this patch, or convert Camera*\n> to shared_ptr in the binding code).\n\nAll these are very good points. I'd propose revisiting this once we move\nforward some more with the bindings.\n\n> > I'll review the patch that makes use of this to see what's best.\n> > \n> >>> Signed-off-by: Tomi Valkeinen \n> >>> ---\n> >>> include/libcamera/request.h | 1 +\n> >>> src/libcamera/request.cpp | 5 +++++\n> >>> 2 files changed, 6 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >>> index 655b1324..f98ef767 100644\n> >>> --- a/include/libcamera/request.h\n> >>> +++ b/include/libcamera/request.h\n> >>> @@ -56,6 +56,7 @@ public:\n> >>> \n> >>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> >>> \n> >>> + std::shared_ptr camera() const;\n> > \n> > Doesn't checkstyle.py warn about spaces uses for indentation ? There's\n> > also a missing blank line.\n> \n> It does, I just didn't run it... Not sure how I managed to indent with spaces, though.\n\ncp utils/hooks/post-commit .git/hooks/\n\n:-)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4B97CBE177\n\tfor ;\n\tMon, 30 Nov 2020 16:00:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2A69634A1;\n\tMon, 30 Nov 2020 17:00:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B9BAC63320\n\tfor ;\n\tMon, 30 Nov 2020 17:00:29 +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 2145FB26;\n\tMon, 30 Nov 2020 17:00:29 +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=\"LujLtVhR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606752029;\n\tbh=8xnbiLD5oOwTj+s1ip6DcOOZI+xLIN0EIJuLGbtg+ug=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LujLtVhR/BXzN92yLfuqe9ITMbc46W8I4Z4UURo6PgLfIDihnZ7k8I4mli4NaFqrj\n\tIZfiS4btf9PjGH/n2By1SKUbjiMFYUm8QSpLkqKbXY38ilaWftSOVmE3jrUuT7pchX\n\tS/65HowQDHgAmlkIzJZtEly2jRM/su4e8f6YNXNs=","Date":"Mon, 30 Nov 2020 18:00:21 +0200","From":"Laurent Pinchart ","To":"Tomi Valkeinen ","Message-ID":"<20201130160021.GB14465@pendragon.ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-3-tomi.valkeinen@iki.fi>\n\t<4ea70c92-aa22-b1c3-be49-a652e6a546af@ideasonboard.com>\n\t<20201129234252.GC5893@pendragon.ideasonboard.com>\n\t<0befc39e-249f-8f1c-4385-8080b7cbca1b@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<0befc39e-249f-8f1c-4385-8080b7cbca1b@iki.fi>","Subject":"Re: [libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request","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":"Tomi Valkeinen ,\n\tlibcamera-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\" "}}]