[{"id":21016,"web_url":"https://patchwork.libcamera.org/comment/21016/","msgid":"","date":"2021-11-19T06:29:43","subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch\n\nOn 11/19/21 5:15 AM, Laurent Pinchart wrote:\n> Template argument deduction results in the lvalue and lvalue reference\n> arguments to the invokeMethod() function causing deduction of the Args\n> template type to a non-reference type. This results in the argument\n> being passed by value and copied.\n\n\nI think this is a documented behavior (and by documented, I mean \nintentional...)\n\n          *\n          * Arguments \\a args passed by value or reference are copied, \nwhile pointers\n          * are passed untouched. The caller shall ensure that any \npointer argument\n          * remains valid until the method is invoked.\n\nShould the documented behavior be tweaked as well?\n\n>\n> Fix this by using a cv-unqualified rvalue reference parameter type. The\n> type is then deduced to an lvalue reference when the argument is an\n\n\nSo does this mean, references can be passed from one thread to another \nin ::invokeMethod() ? Instead of copy-by-value behavior as before? Are \nthere any caveats for doing so?\n\n\n> lvalue or lvalue reference, due to a combination of the special template\n> argument deduction rule for rvalue reference parameter types:\n>\n> If P is an rvalue reference to a cv-unqualified template parameter\n> (so-called forwarding reference), and the corresponding function call\n> argument is an lvalue, the type lvalue reference to A is used in place\n> of A for deduction.\n>\n> (https://en.cppreference.com/w/cpp/language/template_argument_deduction)\n>\n> and the reference collapsing rule\n> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).\n\n\nAs long as we are OK with passing references(lvalue or rvalue) across \nthreads (that's my understanding of use-case of ::invokeMethod()) the \nreference collapsing done here makes sense.\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/object.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n> index 5c385ab4b140..ae2a275caf68 100644\n> --- a/include/libcamera/base/object.h\n> +++ b/include/libcamera/base/object.h\n> @@ -34,7 +34,7 @@ public:\n> \ttemplate \t\t typename std::enable_if_t::value> * = nullptr>\n> \tR invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,\n> -\t\t Args... args)\n> +\t\t Args&&... args)\n> \t{\n> \t\tT *obj = static_cast(this);\n> \t\tauto *method = new BoundMethodMember(obj, this, func, type);\n>\n> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6","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 6DC5DBDB1C\n\tfor ;\n\tFri, 19 Nov 2021 06:29:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E30260371;\n\tFri, 19 Nov 2021 07:29:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5D9A60121\n\tfor ;\n\tFri, 19 Nov 2021 07:29:49 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.26])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D1B35A7;\n\tFri, 19 Nov 2021 07:29:48 +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=\"uatrN/8J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637303389;\n\tbh=YAssB316QwFaltNnPyBc0HLYCkG8M5nS0TS9ml16QuY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=uatrN/8JlJM3LSQ4p0k8pZ0Q18CB/pz+diMwple4kJG4J/WcbdusNA/GqFvH9dFzp\n\t8zsM/cJy+VG49UtJQJloYzSMo572LfWphL6JQOjIzHxM1nNyAwQQdBFlerrF/5B4EI\n\tZlRCdb4SxUnjdZVz5RGyli3PoLWt+x3iLnZKdWj0=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 19 Nov 2021 11:59:43 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21019,"web_url":"https://patchwork.libcamera.org/comment/21019/","msgid":"","date":"2021-11-19T10:30:16","subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Nov 19, 2021 at 11:59:43AM +0530, Umang Jain wrote:\n> On 11/19/21 5:15 AM, Laurent Pinchart wrote:\n> > Template argument deduction results in the lvalue and lvalue reference\n> > arguments to the invokeMethod() function causing deduction of the Args\n> > template type to a non-reference type. This results in the argument\n> > being passed by value and copied.\n> \n> \n> I think this is a documented behavior (and by documented, I mean \n> intentional...)\n> \n>          *\n>          * Arguments \\a args passed by value or reference are copied, \n> while pointers\n>          * are passed untouched. The caller shall ensure that any \n> pointer argument\n>          * remains valid until the method is invoked.\n> \n> Should the documented behavior be tweaked as well?\n\nThere's still a copy, when passing the arguments to ->activate() and\nstoring them in the pack. This patch only avoids one extra copy.\n\n> > Fix this by using a cv-unqualified rvalue reference parameter type. The\n> > type is then deduced to an lvalue reference when the argument is an\n> \n> So does this mean, references can be passed from one thread to another \n> in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are \n> there any caveats for doing so?\n\nThe references can now be passed to invokeMethod(), but still not to the\nnext layer. We still need copies for cross-thread calls, at least when\nthey're asynchronous.\n\n> > lvalue or lvalue reference, due to a combination of the special template\n> > argument deduction rule for rvalue reference parameter types:\n> >\n> > If P is an rvalue reference to a cv-unqualified template parameter\n> > (so-called forwarding reference), and the corresponding function call\n> > argument is an lvalue, the type lvalue reference to A is used in place\n> > of A for deduction.\n> >\n> > (https://en.cppreference.com/w/cpp/language/template_argument_deduction)\n> >\n> > and the reference collapsing rule\n> > (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).\n> \n> As long as we are OK with passing references(lvalue or rvalue) across \n> threads (that's my understanding of use-case of ::invokeMethod()) the \n> reference collapsing done here makes sense.\n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/object.h | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n> > index 5c385ab4b140..ae2a275caf68 100644\n> > --- a/include/libcamera/base/object.h\n> > +++ b/include/libcamera/base/object.h\n> > @@ -34,7 +34,7 @@ public:\n> > \ttemplate > \t\t typename std::enable_if_t::value> * = nullptr>\n> > \tR invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,\n> > -\t\t Args... args)\n> > +\t\t Args&&... args)\n> > \t{\n> > \t\tT *obj = static_cast(this);\n> > \t\tauto *method = new BoundMethodMember(obj, this, func, type);\n> >\n> > base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6","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 5DB6FBF415\n\tfor ;\n\tFri, 19 Nov 2021 10:30:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB7FA60233;\n\tFri, 19 Nov 2021 11:30:41 +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 03F006022F\n\tfor ;\n\tFri, 19 Nov 2021 11:30:40 +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 7CB4D1974;\n\tFri, 19 Nov 2021 11:30:39 +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=\"viczJQFV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637317839;\n\tbh=LiGJpg/NONyIHGvxgHLfdAV8iGW7XrSBPnLoKN0Ur2U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=viczJQFVWSC+h3DgpRkrxhtAnLfqJL6DrsJ1h43ya6T30273nucF7sYAg0pDENX0y\n\t1ONXwgNyGehSwyR4pfrRHKiqJ0EEBUA3f0vDaVgCCEWLUKNbfCxAz/6R7wybXsBPtH\n\tuyguuLyp/TR5RDNcFdYa+OSzFu4M/6nKzikZks9s=","Date":"Fri, 19 Nov 2021 12:30:16 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21030,"web_url":"https://patchwork.libcamera.org/comment/21030/","msgid":"<30a36670-524b-1d36-9a61-6f8dc5b4a34e@ideasonboard.com>","date":"2021-11-19T11:45:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 11/19/21 4:00 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Fri, Nov 19, 2021 at 11:59:43AM +0530, Umang Jain wrote:\n>> On 11/19/21 5:15 AM, Laurent Pinchart wrote:\n>>> Template argument deduction results in the lvalue and lvalue reference\n>>> arguments to the invokeMethod() function causing deduction of the Args\n>>> template type to a non-reference type. This results in the argument\n>>> being passed by value and copied.\n>>\n>> I think this is a documented behavior (and by documented, I mean\n>> intentional...)\n>>\n>>          *\n>>          * Arguments \\a args passed by value or reference are copied,\n>> while pointers\n>>          * are passed untouched. The caller shall ensure that any\n>> pointer argument\n>>          * remains valid until the method is invoked.\n>>\n>> Should the documented behavior be tweaked as well?\n> There's still a copy, when passing the arguments to ->activate() and\n> storing them in the pack. This patch only avoids one extra copy.\n\n\nAh okay, I got confused with the copy itself is happening the \ninvokeMethod() stage (hideous). Indeed we do avoid one extra copy now \nafter I read the invokeMethod() implementation.\n\n>\n>>> Fix this by using a cv-unqualified rvalue reference parameter type. The\n>>> type is then deduced to an lvalue reference when the argument is an\n>> So does this mean, references can be passed from one thread to another\n>> in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are\n>> there any caveats for doing so?\n> The references can now be passed to invokeMethod(), but still not to the\n> next layer. We still need copies for cross-thread calls, at least when\n> they're asynchronous.\n\n\nYes, the documentation is intact, sorry for the noise.\n\nI wonder why we can't send references across threads? But for this patch,\n\nReviewed-by: Umang Jain \n\n>\n>>> lvalue or lvalue reference, due to a combination of the special template\n>>> argument deduction rule for rvalue reference parameter types:\n>>>\n>>> If P is an rvalue reference to a cv-unqualified template parameter\n>>> (so-called forwarding reference), and the corresponding function call\n>>> argument is an lvalue, the type lvalue reference to A is used in place\n>>> of A for deduction.\n>>>\n>>> (https://en.cppreference.com/w/cpp/language/template_argument_deduction)\n>>>\n>>> and the reference collapsing rule\n>>> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).\n>> As long as we are OK with passing references(lvalue or rvalue) across\n>> threads (that's my understanding of use-case of ::invokeMethod()) the\n>> reference collapsing done here makes sense.\n>>\n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> include/libcamera/base/object.h | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n>>> index 5c385ab4b140..ae2a275caf68 100644\n>>> --- a/include/libcamera/base/object.h\n>>> +++ b/include/libcamera/base/object.h\n>>> @@ -34,7 +34,7 @@ public:\n>>> \ttemplate>> \t\t typename std::enable_if_t::value> * = nullptr>\n>>> \tR invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,\n>>> -\t\t Args... args)\n>>> +\t\t Args&&... args)\n>>> \t{\n>>> \t\tT *obj = static_cast(this);\n>>> \t\tauto *method = new BoundMethodMember(obj, this, func, type);\n>>>\n>>> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6","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 CBFE4BF415\n\tfor ;\n\tFri, 19 Nov 2021 11:45:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E9F560378;\n\tFri, 19 Nov 2021 12:45: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 126B66033C\n\tfor ;\n\tFri, 19 Nov 2021 12:45:09 +0100 (CET)","from [192.168.1.106] (unknown [103.238.109.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 140171959;\n\tFri, 19 Nov 2021 12:45:07 +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=\"qXnXXqQ7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637322308;\n\tbh=6d199KHqrR9sZHuZA7tGbLL5mXDrf1HsRFNRdWyJHLE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qXnXXqQ7rTrvjYpLsR58VXw705qIp6mNxq+3gFUZvRmysKkWK5PV3p1yHIyj9Ze3C\n\tqf/tpswdsaopSZSFUl0iRd4EF9sU0iKDzVo6wWEnh3yKXIw2ttwBkUJC7KszRphjkL\n\tDChdKUW1G+zDy8HOQ8EbCy4Xa/3t4D3F3yJhpXXs=","To":"Laurent Pinchart ","References":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"<30a36670-524b-1d36-9a61-6f8dc5b4a34e@ideasonboard.com>","Date":"Fri, 19 Nov 2021 17:15:02 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21032,"web_url":"https://patchwork.libcamera.org/comment/21032/","msgid":"","date":"2021-11-19T11:50:36","subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Nov 19, 2021 at 05:15:02PM +0530, Umang Jain wrote:\n> On 11/19/21 4:00 PM, Laurent Pinchart wrote:\n> > On Fri, Nov 19, 2021 at 11:59:43AM +0530, Umang Jain wrote:\n> >> On 11/19/21 5:15 AM, Laurent Pinchart wrote:\n> >>> Template argument deduction results in the lvalue and lvalue reference\n> >>> arguments to the invokeMethod() function causing deduction of the Args\n> >>> template type to a non-reference type. This results in the argument\n> >>> being passed by value and copied.\n> >>\n> >> I think this is a documented behavior (and by documented, I mean\n> >> intentional...)\n> >>\n> >>          *\n> >>          * Arguments \\a args passed by value or reference are copied, while pointers\n> >>          * are passed untouched. The caller shall ensure that any pointer argument\n> >>          * remains valid until the method is invoked.\n> >>\n> >> Should the documented behavior be tweaked as well?\n> >\n> > There's still a copy, when passing the arguments to ->activate() and\n> > storing them in the pack. This patch only avoids one extra copy.\n> \n> Ah okay, I got confused with the copy itself is happening the \n> invokeMethod() stage (hideous). Indeed we do avoid one extra copy now \n> after I read the invokeMethod() implementation.\n> \n> >>> Fix this by using a cv-unqualified rvalue reference parameter type. The\n> >>> type is then deduced to an lvalue reference when the argument is an\n> >> So does this mean, references can be passed from one thread to another\n> >> in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are\n> >> there any caveats for doing so?\n> >\n> > The references can now be passed to invokeMethod(), but still not to the\n> > next layer. We still need copies for cross-thread calls, at least when\n> > they're asynchronous.\n> \n> Yes, the documentation is intact, sorry for the noise.\n> \n> I wonder why we can't send references across threads? But for this patch,\n\nWe can, but it's dangerous. For asynchronous calls, the caller would\nneed to guarantee that the reference stays valid until the asynchronous\ncall finishes. It's possible in some cases, but would very likely be a\nsource of hard to debug issues.\n\nFor synchronous calls, we could store references in the parameters pack.\nThat's a possible optimization candidate for later.\n\n> Reviewed-by: Umang Jain \n> \n> >>> lvalue or lvalue reference, due to a combination of the special template\n> >>> argument deduction rule for rvalue reference parameter types:\n> >>>\n> >>> If P is an rvalue reference to a cv-unqualified template parameter\n> >>> (so-called forwarding reference), and the corresponding function call\n> >>> argument is an lvalue, the type lvalue reference to A is used in place\n> >>> of A for deduction.\n> >>>\n> >>> (https://en.cppreference.com/w/cpp/language/template_argument_deduction)\n> >>>\n> >>> and the reference collapsing rule\n> >>> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).\n> >>\n> >> As long as we are OK with passing references(lvalue or rvalue) across\n> >> threads (that's my understanding of use-case of ::invokeMethod()) the\n> >> reference collapsing done here makes sense.\n> >>\n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> include/libcamera/base/object.h | 2 +-\n> >>> 1 file changed, 1 insertion(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n> >>> index 5c385ab4b140..ae2a275caf68 100644\n> >>> --- a/include/libcamera/base/object.h\n> >>> +++ b/include/libcamera/base/object.h\n> >>> @@ -34,7 +34,7 @@ public:\n> >>> \ttemplate >>> \t\t typename std::enable_if_t::value> * = nullptr>\n> >>> \tR invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,\n> >>> -\t\t Args... args)\n> >>> +\t\t Args&&... args)\n> >>> \t{\n> >>> \t\tT *obj = static_cast(this);\n> >>> \t\tauto *method = new BoundMethodMember(obj, this, func, type);\n> >>>\n> >>> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6","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 8E0A4BDB13\n\tfor ;\n\tFri, 19 Nov 2021 11:51:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D71C360398;\n\tFri, 19 Nov 2021 12:51: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 423C260371\n\tfor ;\n\tFri, 19 Nov 2021 12:51:00 +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 A5B661959;\n\tFri, 19 Nov 2021 12:50: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=\"k839HkXe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637322659;\n\tbh=6A5O70GDYuMCBcohgJ70uMixSMAx+q39ozZRSCEue98=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k839HkXe9aD86aK+oHDvrfjeJVehq0prpSmjQr2EUL8iVjKLilKBJlE2u86k0xU72\n\tE5+dhvrtByMoNa6f4hpJ1uF+V4FeqSx50RG4+BAuWA8gZ/dDEUM3xFGAJZ7TQ5AYnq\n\tMXUAAARJ+0xOkIxRjrUhzJJowK4qpnYYsv3szFWI=","Date":"Fri, 19 Nov 2021 13:50:36 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<30a36670-524b-1d36-9a61-6f8dc5b4a34e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<30a36670-524b-1d36-9a61-6f8dc5b4a34e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21193,"web_url":"https://patchwork.libcamera.org/comment/21193/","msgid":"<163775505625.3059017.83076217107485600@Monstersaurus>","date":"2021-11-24T11:57:36","subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-11-19 11:50:36)\n> Hi Umang,\n> \n> On Fri, Nov 19, 2021 at 05:15:02PM +0530, Umang Jain wrote:\n> > On 11/19/21 4:00 PM, Laurent Pinchart wrote:\n> > > On Fri, Nov 19, 2021 at 11:59:43AM +0530, Umang Jain wrote:\n> > >> On 11/19/21 5:15 AM, Laurent Pinchart wrote:\n> > >>> Template argument deduction results in the lvalue and lvalue reference\n> > >>> arguments to the invokeMethod() function causing deduction of the Args\n> > >>> template type to a non-reference type. This results in the argument\n> > >>> being passed by value and copied.\n> > >>\n> > >> I think this is a documented behavior (and by documented, I mean\n> > >> intentional...)\n> > >>\n> > >>          *\n> > >>          * Arguments \\a args passed by value or reference are copied, while pointers\n> > >>          * are passed untouched. The caller shall ensure that any pointer argument\n> > >>          * remains valid until the method is invoked.\n> > >>\n> > >> Should the documented behavior be tweaked as well?\n> > >\n> > > There's still a copy, when passing the arguments to ->activate() and\n> > > storing them in the pack. This patch only avoids one extra copy.\n> > \n> > Ah okay, I got confused with the copy itself is happening the \n> > invokeMethod() stage (hideous). Indeed we do avoid one extra copy now \n> > after I read the invokeMethod() implementation.\n> > \n> > >>> Fix this by using a cv-unqualified rvalue reference parameter type. The\n> > >>> type is then deduced to an lvalue reference when the argument is an\n> > >> So does this mean, references can be passed from one thread to another\n> > >> in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are\n> > >> there any caveats for doing so?\n> > >\n> > > The references can now be passed to invokeMethod(), but still not to the\n> > > next layer. We still need copies for cross-thread calls, at least when\n> > > they're asynchronous.\n> > \n> > Yes, the documentation is intact, sorry for the noise.\n> > \n> > I wonder why we can't send references across threads? But for this patch,\n> \n> We can, but it's dangerous. For asynchronous calls, the caller would\n> need to guarantee that the reference stays valid until the asynchronous\n> call finishes. It's possible in some cases, but would very likely be a\n> source of hard to debug issues.\n> \n> For synchronous calls, we could store references in the parameters pack.\n> That's a possible optimization candidate for later.\n> \n> > Reviewed-by: Umang Jain \n\nReviewed-by: Kieran Bingham \n\n> > \n> > >>> lvalue or lvalue reference, due to a combination of the special template\n> > >>> argument deduction rule for rvalue reference parameter types:\n> > >>>\n> > >>> If P is an rvalue reference to a cv-unqualified template parameter\n> > >>> (so-called forwarding reference), and the corresponding function call\n> > >>> argument is an lvalue, the type lvalue reference to A is used in place\n> > >>> of A for deduction.\n> > >>>\n> > >>> (https://en.cppreference.com/w/cpp/language/template_argument_deduction)\n> > >>>\n> > >>> and the reference collapsing rule\n> > >>> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).\n> > >>\n> > >> As long as we are OK with passing references(lvalue or rvalue) across\n> > >> threads (that's my understanding of use-case of ::invokeMethod()) the\n> > >> reference collapsing done here makes sense.\n> > >>\n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> ---\n> > >>> include/libcamera/base/object.h | 2 +-\n> > >>> 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n> > >>> index 5c385ab4b140..ae2a275caf68 100644\n> > >>> --- a/include/libcamera/base/object.h\n> > >>> +++ b/include/libcamera/base/object.h\n> > >>> @@ -34,7 +34,7 @@ public:\n> > >>> template > >>> typename std::enable_if_t::value> * = nullptr>\n> > >>> R invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,\n> > >>> - Args... args)\n> > >>> + Args&&... args)\n> > >>> {\n> > >>> T *obj = static_cast(this);\n> > >>> auto *method = new BoundMethodMember(obj, this, func, type);\n> > >>>\n> > >>> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 2779CBDB13\n\tfor ;\n\tWed, 24 Nov 2021 11:57:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9238B60371;\n\tWed, 24 Nov 2021 12:57:39 +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 C720860128\n\tfor ;\n\tWed, 24 Nov 2021 12:57:38 +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 57E9CD78;\n\tWed, 24 Nov 2021 12:57:38 +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=\"cjl7wZJ9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637755058;\n\tbh=1iwYwGHDQEWLDpWQZWghm6yPtByCte2lGIr0S4bgtao=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cjl7wZJ9xAPq1TaQfeo2m5/THoui9A+BWlxlxEHA+65uGtqBjwrs5OHOnKzphsPbi\n\tPXusYiMTrrRu4xEiSTu/nZPvX1+J/8w5oKlZyCWyALv2i9YgskTyvcr3HrYs7nVSW/\n\tfj2kUHnb5a8BjDPuAwBndSrHMEK0gkOM8Ool4fsc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211118234546.9848-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<30a36670-524b-1d36-9a61-6f8dc5b4a34e@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Wed, 24 Nov 2021 11:57:36 +0000","Message-ID":"<163775505625.3059017.83076217107485600@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: object: Avoid argument\n\tcopies in invokeMethod()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]