[{"id":33558,"web_url":"https://patchwork.libcamera.org/comment/33558/","msgid":"<20250303222628.GD30583@pendragon.ideasonboard.com>","date":"2025-03-03T22:26:28","subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote:\n> Take the unique pointer to the `Fence` object by rvalue reference\n> so that it is not destroyed if the function returns an error code\n> and does not take ownership of the unique pointer.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/request.h | 2 +-\n> src/libcamera/request.cpp | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index e214a9d13..0c5939f7b 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -53,7 +53,7 @@ public:\n> \tControlList &metadata() { return *metadata_; }\n> \tconst BufferMap &buffers() const { return bufferMap_; }\n> \tint addBuffer(const Stream *stream, FrameBuffer *buffer,\n> -\t\t std::unique_ptr fence = nullptr);\n> +\t\t std::unique_ptr &&fence = {});\n\nThere's one caller in src/android/camera_device.cpp that passes nullptr\nas the third argument. Could you fix that ?\n\nThe other caller in the same file creates the unique_ptr right before\ncalling the function, so the fence will be destroyed anyway. I suppose\nthat's fine for now.\n\nShould the unit test also be extended to validate the new behaviour ?\nThe function documentation should also be updated.\n\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> \tuint32_t sequence() const;\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index b206ac132..7d02f09fd 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags)\n> * \\retval -EINVAL The buffer does not reference a valid Stream\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> -\t\t std::unique_ptr fence)\n> +\t\t std::unique_ptr &&fence)\n> {\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";","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 0C27DBD808\n\tfor ;\n\tMon, 3 Mar 2025 22:26:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1990468772;\n\tMon, 3 Mar 2025 23:26:52 +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 149FC68772\n\tfor ;\n\tMon, 3 Mar 2025 23:26:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DB6EF6;\n\tMon, 3 Mar 2025 23:25: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=\"JsoGnPN1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741040718;\n\tbh=nGZA1DlNdDBqf/xq9vXFV+PswAl5cFJwEFDyEPK/PsY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JsoGnPN1CLraYWkNMcIFE3oUjo894rgAL+amGfBG8Kp66b4k2j2FKNoVFWCRg56Tx\n\tjf7lhtfoqjCYu+95KjFdEfPQU61h5vRsHPoJkY+cZneXf6VcGsIzjTQ9Lveq0rbS3+\n\ttYVAeeDz6OXmb2iSZuSQX7da7jcqxKPJz7CfEI4o=","Date":"Tue, 4 Mar 2025 00:26:28 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","Message-ID":"<20250303222628.GD30583@pendragon.ideasonboard.com>","References":"<20250303151634.734176-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250303151634.734176-1-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33571,"web_url":"https://patchwork.libcamera.org/comment/33571/","msgid":"","date":"2025-03-04T13:14:05","subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 03. 23:26 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote:\n>> Take the unique pointer to the `Fence` object by rvalue reference\n>> so that it is not destroyed if the function returns an error code\n>> and does not take ownership of the unique pointer.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/request.h | 2 +-\n>> src/libcamera/request.cpp | 2 +-\n>> 2 files changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index e214a9d13..0c5939f7b 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -53,7 +53,7 @@ public:\n>> \tControlList &metadata() { return *metadata_; }\n>> \tconst BufferMap &buffers() const { return bufferMap_; }\n>> \tint addBuffer(const Stream *stream, FrameBuffer *buffer,\n>> -\t\t std::unique_ptr fence = nullptr);\n>> +\t\t std::unique_ptr &&fence = {});\n> \n> There's one caller in src/android/camera_device.cpp that passes nullptr\n> as the third argument. Could you fix that ?\n\nIt does not cause any issues, but I'll add a separate commit to change it.\n\n\n> \n> The other caller in the same file creates the unique_ptr right before\n> calling the function, so the fence will be destroyed anyway. I suppose\n> that's fine for now.\n\nThere is no change in behaviour in case of success, and in case of failure\nthere is minimal difference in most cases. No in-tree users are affected\nas far as I could tell.\n\n\n> \n> Should the unit test also be extended to validate the new behaviour ?\n\nI can add some extra checks to `test/fence.cpp` if that's fine?\n\n\n> The function documentation should also be updated.\n\nACK\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> \tFrameBuffer *findBuffer(const Stream *stream) const;\n>> \n>> \tuint32_t sequence() const;\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index b206ac132..7d02f09fd 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags)\n>> * \\retval -EINVAL The buffer does not reference a valid Stream\n>> */\n>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n>> -\t\t std::unique_ptr fence)\n>> +\t\t std::unique_ptr &&fence)\n>> {\n>> \tif (!stream) {\n>> \t\tLOG(Request, Error) << \"Invalid stream reference\";\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 29C7EC3257\n\tfor ;\n\tTue, 4 Mar 2025 13:14:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0500F687F0;\n\tTue, 4 Mar 2025 14:14:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33EBE68755\n\tfor ;\n\tTue, 4 Mar 2025 14:14:09 +0100 (CET)","from [192.168.33.3] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DED778FA;\n\tTue, 4 Mar 2025 14:12:36 +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=\"u5+EhCNq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741093957;\n\tbh=mcEZVVUb5FSRL+T8Nod7l3SfoE7WViwqHfwQ76U0jJg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=u5+EhCNqTpcScplIu3cfqfEa3V8yNmUIo2s6jyxlI1S8OgSU7FpYjx6gyrRbXchWm\n\tY8nhCSEscFHLFF0JY8jUFLGemDbn+OugPSJWMXugMJP9yyQ4e8wTUD1GGOh/W8rLSQ\n\tpzHxCiBbCWdSQbZ+R7snyZHqLuvQUDbBX0bDQ2AY=","Message-ID":"","Date":"Tue, 4 Mar 2025 14:14:05 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303151634.734176-1-barnabas.pocze@ideasonboard.com>\n\t<20250303222628.GD30583@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250303222628.GD30583@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34405,"web_url":"https://patchwork.libcamera.org/comment/34405/","msgid":"<31beefa2-1409-46d8-b900-10eaa4e87950@ideasonboard.com>","date":"2025-06-04T07:52:59","subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 03. 04. 14:14 keltezéssel, Barnabás Pőcze írta:\n> Hi\n> \n> \n> 2025. 03. 03. 23:26 keltezéssel, Laurent Pinchart írta:\n>> Hi Barnabás,\n>>\n>> Thank you for the patch.\n>>\n>> On Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote:\n>>> Take the unique pointer to the `Fence` object by rvalue reference\n>>> so that it is not destroyed if the function returns an error code\n>>> and does not take ownership of the unique pointer.\n>>>\n>>> Signed-off-by: Barnabás Pőcze \n>>> ---\n>>> include/libcamera/request.h | 2 +-\n>>> src/libcamera/request.cpp | 2 +-\n>>> 2 files changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>>> index e214a9d13..0c5939f7b 100644\n>>> --- a/include/libcamera/request.h\n>>> +++ b/include/libcamera/request.h\n>>> @@ -53,7 +53,7 @@ public:\n>>> \tControlList &metadata() { return *metadata_; }\n>>> \tconst BufferMap &buffers() const { return bufferMap_; }\n>>> \tint addBuffer(const Stream *stream, FrameBuffer *buffer,\n>>> -\t\t std::unique_ptr fence = nullptr);\n>>> +\t\t std::unique_ptr &&fence = {});\n>>\n>> There's one caller in src/android/camera_device.cpp that passes nullptr\n>> as the third argument. Could you fix that ?\n> \n> It does not cause any issues, but I'll add a separate commit to change it.\n\nDone: https://gitlab.freedesktop.org/camera/libcamera/-/commit/633063e099ccb0618359f9ecde9b05852c34259d\n\n\n> \n> \n>>\n>> The other caller in the same file creates the unique_ptr right before\n>> calling the function, so the fence will be destroyed anyway. I suppose\n>> that's fine for now.\n> \n> There is no change in behaviour in case of success, and in case of failure\n> there is minimal difference in most cases. No in-tree users are affected\n> as far as I could tell.\n> \n> \n>>\n>> Should the unit test also be extended to validate the new behaviour ?\n> \n> I can add some extra checks to `test/fence.cpp` if that's fine?\n\nIs the above fine or did you have something else in mind?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> \n>> The function documentation should also be updated.\n> \n> ACK\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n>>\n>>> \tFrameBuffer *findBuffer(const Stream *stream) const;\n>>>\n>>> \tuint32_t sequence() const;\n>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>>> index b206ac132..7d02f09fd 100644\n>>> --- a/src/libcamera/request.cpp\n>>> +++ b/src/libcamera/request.cpp\n>>> @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags)\n>>> * \\retval -EINVAL The buffer does not reference a valid Stream\n>>> */\n>>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n>>> -\t\t std::unique_ptr fence)\n>>> +\t\t std::unique_ptr &&fence)\n>>> {\n>>> \tif (!stream) {\n>>> \t\tLOG(Request, Error) << \"Invalid stream reference\";\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 5B4EAC31E9\n\tfor ;\n\tWed, 4 Jun 2025 07:53:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C5FF68DB3;\n\tWed, 4 Jun 2025 09:53:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F266368DB0\n\tfor ;\n\tWed, 4 Jun 2025 09:53:02 +0200 (CEST)","from [192.168.33.16] (185.182.214.140.nat.pool.zt.hu\n\t[185.182.214.140])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23D5378C;\n\tWed, 4 Jun 2025 09:53:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JSnmOipH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749023580;\n\tbh=36hk4XIqz+RHFlyPW4coZkBIVGq1LBiSQ3fgFtzyT7A=;\n\th=Date:Subject:From:To:Cc:Reply-To:References:In-Reply-To:From;\n\tb=JSnmOipHM0IuHbFUNlG79ka1jRV3gUWSPwEwNd2vwp2VCC8disUioIy2Me1pq1pPA\n\t6tuAOhqeYYS2xM2BUDwu+tBtt1+5a72EjFLegmSkSP3aBhvfHzTzC3DKKf+PVctdhp\n\tke8tZuLZO6iZEXkYATwHMMv/BRmp9v/Hj+xbLa3k=","Message-ID":"<31beefa2-1409-46d8-b900-10eaa4e87950@ideasonboard.com>","Date":"Wed, 4 Jun 2025 09:52:59 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: request: addBuffer(): Do not destroy fence\n\ton failure","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303151634.734176-1-barnabas.pocze@ideasonboard.com>\n\t<20250303222628.GD30583@pendragon.ideasonboard.com>\n\t<8OG6NZjsU_yD3h7Ll-ZD7fDXu5e29-3x6q5LrEqQVrAicXGO4Z1Wlamsk5Lnn25RjsyPTr07-jBvqm7YVmj7mw==@protonmail.internalid>\n\t","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]