[{"id":32951,"web_url":"https://patchwork.libcamera.org/comment/32951/","msgid":"","date":"2025-01-07T17:08:24","subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabas\n\nOn Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote:\n> Do a consistency check to ensure that the return value\n> matches the number of buffers actually created.\n\nCould you please make sure your commit messages in the series span to\nup to 72 cols ?\n\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index b473e0773..5cc7635f7 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -49,6 +49,7 @@ void Capture::start()\n>\n> \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> \tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> +\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n\nmmm, FrameBufferAllocator::allocate() returns the number of allocated\nbuffers, if this doesn't match\n\n allocator_.buffers(stream).size())\n\nit's likely a problem in the FrameBufferAllocator implementation\nrather than an issue here.\n\nUnless I missed something, I would drop this patch\n\n>\n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> --\n> 2.47.1\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 7D648C32DC\n\tfor ;\n\tTue, 7 Jan 2025 17:08:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B715684E4;\n\tTue, 7 Jan 2025 18:08:30 +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 26673684CD\n\tfor ;\n\tTue, 7 Jan 2025 18:08:29 +0100 (CET)","from ideasonboard.com (mob-5-90-140-128.net.vodafone.it\n\t[5.90.140.128])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ACA14496;\n\tTue, 7 Jan 2025 18:07: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=\"cMh9AI/5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736269656;\n\tbh=Mx9EBVh4xHnz0CPIVrRYD+MtznmDQFf+4wC8JaTVILQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cMh9AI/5Szu2Rs/4FXEDAKFVWlIbIIJa4QIPQX91OLpJkVTyh8AcTDerlEtWaJ4dw\n\t1yF/jwjm+xt69mRe6JblLA/ZLsZQSRoeWRhRFQBTXvKfUpDwoYhqU64ad3zqkyAqmu\n\tDv96XKrW0+ImnUY/roUv3Iob8bvAyk590fjjSfoo=","Date":"Tue, 7 Jan 2025 18:08:24 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","Message-ID":"","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-10-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241220150759.709756-10-pobrn@protonmail.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":33013,"web_url":"https://patchwork.libcamera.org/comment/33013/","msgid":"","date":"2025-01-10T09:46:23","subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabas\n> \n> On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote:\n> > Do a consistency check to ensure that the return value\n> > matches the number of buffers actually created.\n> \n> Could you please make sure your commit messages in the series span to\n> up to 72 cols ?\n> \n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index b473e0773..5cc7635f7 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -49,6 +49,7 @@ void Capture::start()\n> >\n> > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > \tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > +\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> \n> mmm, FrameBufferAllocator::allocate() returns the number of allocated\n> buffers, if this doesn't match\n> \n> allocator_.buffers(stream).size())\n> \n> it's likely a problem in the FrameBufferAllocator implementation\n> rather than an issue here.\n\nI think there should be some kind of a consistency check either in\n`FrameBufferAllocator::allocate()` or at least here.\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Unless I missed something, I would drop this patch\n> \n> >\n> > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > --\n> > 2.47.1\n> >\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 4A0B4C32EF\n\tfor ;\n\tFri, 10 Jan 2025 09:46:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20AD6684EA;\n\tFri, 10 Jan 2025 10:46:29 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6F7661882\n\tfor ;\n\tFri, 10 Jan 2025 10:46:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"EA4AWNlI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736502386; x=1736761586;\n\tbh=PCI5awxBfCwZtxcIHFgK6Aqx1j7AvaibjVPeHYBC03E=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=EA4AWNlII7xprg+QfsdQwijvSQFDLI13W5WVZwMWBRHf/XOLLdCG5d321RZ2SB/1r\n\tzQTDMQyBFCY2EOdeJOrfdp5TvW9LsykRrLxdNajBxzQfI8QiDM2Kmkbv6HBXvxD/6Y\n\tn4x86MNvDDxs4y621zZ0sQ39IHiQ99z3OlBMc7jrDsHX4Ojnib+hMW1rJb4OySXMxl\n\tAkEJb9fK1GuI7cJ4e6jbKzMr1pc2xnyXJD3lrX8VzMaQsvlvkwofDIdodDSeu0zEXa\n\tVjf1jTV2VjkjH0UPm0wYY3n7fdZcBsP8+4U6qbHR+rK/C3Y153ybyNfoxW7aKDK2kf\n\t/XBWaqZheLOMA==","Date":"Fri, 10 Jan 2025 09:46:23 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","Message-ID":"","In-Reply-To":"","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-10-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"78a52949acd3ea2ccacc99f08ee7ded77b61cc74","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":33017,"web_url":"https://patchwork.libcamera.org/comment/33017/","msgid":"<20250110105258.GA8484@pendragon.ideasonboard.com>","date":"2025-01-10T10:52:58","subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote:\n> 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta:\n> > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote:\n> > > Do a consistency check to ensure that the return value\n> > > matches the number of buffers actually created.\n> > \n> > Could you please make sure your commit messages in the series span to\n> > up to 72 cols ?\n> > \n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index b473e0773..5cc7635f7 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -49,6 +49,7 @@ void Capture::start()\n> > >\n> > > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > \tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > +\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > \n> > mmm, FrameBufferAllocator::allocate() returns the number of allocated\n> > buffers, if this doesn't match\n> > \n> > allocator_.buffers(stream).size())\n> > \n> > it's likely a problem in the FrameBufferAllocator implementation\n> > rather than an issue here.\n> \n> I think there should be some kind of a consistency check either in\n> `FrameBufferAllocator::allocate()` or at least here.\n\nMaybe it's a candidae for a unit test instead ?\n\n> > Unless I missed something, I would drop this patch\n> > \n> > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);","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 4AA6CC32F5\n\tfor ;\n\tFri, 10 Jan 2025 10:53:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28906684E4;\n\tFri, 10 Jan 2025 11:53: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 5186D61882\n\tfor ;\n\tFri, 10 Jan 2025 11:53:02 +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 CDED378C;\n\tFri, 10 Jan 2025 11:52: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=\"WatmqWzy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736506328;\n\tbh=DAn+MG6bRLt37YQR/SYxpTqpRHGuz3WoezxKlmz1btM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WatmqWzyDbdM2+swknqLqmu+PE3bgmDx8CpG7haOux9g+LV/yMENmVvRhw+V0GOlh\n\tP1tvJvHuvJ6i64zGYla9yMAVZDfeQiJBw4zLg++iYD+zQRgGNVFuyGVAjOpcHm0cKH\n\thsOjo7kxiok0O9XENA5p5ucEOK6vA7tnTeHNiMQU=","Date":"Fri, 10 Jan 2025 12:52:58 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","Message-ID":"<20250110105258.GA8484@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-10-pobrn@protonmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":33018,"web_url":"https://patchwork.libcamera.org/comment/33018/","msgid":"","date":"2025-01-10T12:15:52","subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 10., péntek 11:52 keltezéssel, Laurent Pinchart írta:\n\n> On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote:\n> > 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta:\n> > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote:\n> > > > Do a consistency check to ensure that the return value\n> > > > matches the number of buffers actually created.\n> > >\n> > > Could you please make sure your commit messages in the series span to\n> > > up to 72 cols ?\n> > >\n> > > > Signed-off-by: Barnabás Pőcze \n> > > > ---\n> > > > src/apps/lc-compliance/helpers/capture.cpp | 1 +\n> > > > 1 file changed, 1 insertion(+)\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > index b473e0773..5cc7635f7 100644\n> > > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > @@ -49,6 +49,7 @@ void Capture::start()\n> > > >\n> > > > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > > \tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > > +\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > >\n> > > mmm, FrameBufferAllocator::allocate() returns the number of allocated\n> > > buffers, if this doesn't match\n> > >\n> > > allocator_.buffers(stream).size())\n> > >\n> > > it's likely a problem in the FrameBufferAllocator implementation\n> > > rather than an issue here.\n> >\n> > I think there should be some kind of a consistency check either in\n> > `FrameBufferAllocator::allocate()` or at least here.\n> \n> Maybe it's a candidae for a unit test instead ?\n\nSorry, but I don't quite see what you mean. I was thinking along the lines of\n\ndiff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\nindex 3d53bde21..7f26e36b9 100644\n--- a/src/libcamera/framebuffer_allocator.cpp\n+++ b/src/libcamera/framebuffer_allocator.cpp\n@@ -101,6 +101,8 @@ int FrameBufferAllocator::allocate(Stream *stream)\n if (ret < 0)\n buffers_.erase(it);\n \n+ ASSERT(std::size_t(ret) == it->second.size());\n+\n return ret;\n }\n \n\nor\n\ndiff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\nindex 3d53bde21..830c6fa1b 100644\n--- a/src/libcamera/framebuffer_allocator.cpp\n+++ b/src/libcamera/framebuffer_allocator.cpp\n@@ -101,6 +101,15 @@ int FrameBufferAllocator::allocate(Stream *stream)\n if (ret < 0)\n buffers_.erase(it);\n \n+ if (std::size_t(ret) != it->second.size()) {\n+ LOG(Allocator, Error)\n+ << \"Pipeline handler bug: \"\n+ << \"return value does not reflect number of allocated buffers: \"\n+ << \"adjusting \" << ret << \" to \" << it->second.size();\n+\n+ ret = it->second.size();\n+ }\n+\n return ret;\n }\n \n\nor possibly\n\ndiff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\nindex 3d53bde21..b583dbbc3 100644\n--- a/src/libcamera/framebuffer_allocator.cpp\n+++ b/src/libcamera/framebuffer_allocator.cpp\n@@ -98,10 +98,12 @@ int FrameBufferAllocator::allocate(Stream *stream)\n << \"Stream is not part of \" << camera_->id()\n << \" active configuration\";\n \n- if (ret < 0)\n+ if (ret < 0) {\n buffers_.erase(it);\n+ return ret;\n+ }\n \n- return ret;\n+ return it->second.size();\n }\n \n /**\n\n\nI am not sure how a unit test could provide the same guarantees.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> > > Unless I missed something, I would drop this patch\n> > >\n> > > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 F0AD6C32EF\n\tfor ;\n\tFri, 10 Jan 2025 12:16:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CF94684E7;\n\tFri, 10 Jan 2025 13:16:00 +0100 (CET)","from mail-10631.protonmail.ch (mail-10631.protonmail.ch\n\t[79.135.106.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 550F2608AA\n\tfor ;\n\tFri, 10 Jan 2025 13:15:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"JLfGZMFX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736511357; x=1736770557;\n\tbh=zczK/RS8ZWKpG3+ixRE+fgvOsxVBJc4lEFvaCKqgw/c=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=JLfGZMFX4RcdWQwskLv1nlFxI2cgwTWfAVmqydq8L/sjp6P/ptNOtZl5jYgQliYhk\n\tdajKUjT4vJW2eGFRIaZKxYA3kWxz2eBSGp+axORy2LBM/dhkBjese5HjR+pvupKO7Z\n\txS/4PO0gaFcrr/T4Pzjx8foJYBQI5kIYjJKKQ4ZTlotZtGtOxqtdFYpa2Qv/QVL0lE\n\tvGqmbZQsqiWF5L8jfINGot3JN4+FEmx84XWNFQa4qrHnE9pWql0mjm0TiwtH5X6VVi\n\tQ221mfi5K+EcpxCj8zI5O1PC622cUzC7nRTheBZA7fYMG4O9PWU+25tEwSsU5YzZWR\n\tPbHS1shsPTNOw==","Date":"Fri, 10 Jan 2025 12:15:52 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","Message-ID":"","In-Reply-To":"<20250110105258.GA8484@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-10-pobrn@protonmail.com>\n\t\n\t\n\t<20250110105258.GA8484@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"b8555f5a4b72d887008cc3d6447dc46374b22698","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":33022,"web_url":"https://patchwork.libcamera.org/comment/33022/","msgid":"<20250110154947.GE7733@pendragon.ideasonboard.com>","date":"2025-01-10T15:49:47","subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jan 10, 2025 at 12:15:52PM +0000, Barnabás Pőcze wrote:\n> 2025. január 10., péntek 11:52 keltezéssel, Laurent Pinchart írta:\n> > On Fri, Jan 10, 2025 at 09:46:23AM +0000, Barnabás Pőcze wrote:\n> > > 2025. január 7., kedd 18:08 keltezéssel, Jacopo Mondi írta:\n> > > > On Fri, Dec 20, 2024 at 03:08:41PM +0000, Barnabás Pőcze wrote:\n> > > > > Do a consistency check to ensure that the return value\n> > > > > matches the number of buffers actually created.\n> > > >\n> > > > Could you please make sure your commit messages in the series span to\n> > > > up to 72 cols ?\n> > > >\n> > > > > Signed-off-by: Barnabás Pőcze \n> > > > > ---\n> > > > > src/apps/lc-compliance/helpers/capture.cpp | 1 +\n> > > > > 1 file changed, 1 insertion(+)\n> > > > >\n> > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > index b473e0773..5cc7635f7 100644\n> > > > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > @@ -49,6 +49,7 @@ void Capture::start()\n> > > > >\n> > > > > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > > > \tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > > > +\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > > >\n> > > > mmm, FrameBufferAllocator::allocate() returns the number of allocated\n> > > > buffers, if this doesn't match\n> > > >\n> > > > allocator_.buffers(stream).size())\n> > > >\n> > > > it's likely a problem in the FrameBufferAllocator implementation\n> > > > rather than an issue here.\n> > >\n> > > I think there should be some kind of a consistency check either in\n> > > `FrameBufferAllocator::allocate()` or at least here.\n> > \n> > Maybe it's a candidae for a unit test instead ?\n> \n> Sorry, but I don't quite see what you mean. I was thinking along the lines of\n> \n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 3d53bde21..7f26e36b9 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -101,6 +101,8 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> if (ret < 0)\n> buffers_.erase(it);\n> \n> + ASSERT(std::size_t(ret) == it->second.size());\n> +\n> return ret;\n> }\n> \n> \n> or\n> \n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 3d53bde21..830c6fa1b 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -101,6 +101,15 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> if (ret < 0)\n> buffers_.erase(it);\n> \n> + if (std::size_t(ret) != it->second.size()) {\n> + LOG(Allocator, Error)\n> + << \"Pipeline handler bug: \"\n> + << \"return value does not reflect number of allocated buffers: \"\n> + << \"adjusting \" << ret << \" to \" << it->second.size();\n> +\n> + ret = it->second.size();\n> + }\n> +\n> return ret;\n> }\n> \n> \n> or possibly\n> \n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 3d53bde21..b583dbbc3 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -98,10 +98,12 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> << \"Stream is not part of \" << camera_->id()\n> << \" active configuration\";\n> \n> - if (ret < 0)\n> + if (ret < 0) {\n> buffers_.erase(it);\n> + return ret;\n> + }\n> \n> - return ret;\n> + return it->second.size();\n> }\n> \n> /**\n> \n> \n> I am not sure how a unit test could provide the same guarantees.\n\nI see what you mean now.\n\nIt seems to me we may be facing an API design issue. The function\nreturns the number of allocated buffers in two different ways, through\nthe return value, and through the buffers vector. If we changed the API\nto return 0 on success, we would get rid of the issue in the first\nplace.\n\nIf we don't want to change the API, and if we're concerned that some\npipeline handlers would implement this incorrectly, we could make\nPipelineHandler::exportFrameBuffers() return 0 on success, and return\nbuffers->size() from Camera::exportFrameBuffers(). That way the ret ==\nbuffers->size() invariant will be implemented in a single place.\n\n> > > > Unless I missed something, I would drop this patch\n> > > >\n> > > > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);","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 C0641C32F5\n\tfor ;\n\tFri, 10 Jan 2025 15:49:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFFDB684E7;\n\tFri, 10 Jan 2025 16:49: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 754C7608AA\n\tfor ;\n\tFri, 10 Jan 2025 16:49:51 +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 C1CCB22E;\n\tFri, 10 Jan 2025 16:48:56 +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=\"vt2HR4gx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736524137;\n\tbh=TPhJclm4CA2+uWyYmMHk/Ol81nxQS7C9hWgPcuzh8pY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vt2HR4gxtQCeThbzxDcJrAKFnTyeB2CRa8X5fG9GJs11m2c5rLsKXVNjm/hVv+BNh\n\tWWkRQY9njMeT2jitkgxMJVZRdg2HHZL5j1NcN+GbMskwC6Oel8z9mwZ11QVj3zLNuZ\n\trJtbw6yb9If34lxEv/7yYPDH2wL4xU5I+2XySAP0=","Date":"Fri, 10 Jan 2025 17:49:47 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 09/12] apps: lc-compliance: Check number of\n\tbuffers in allocator","Message-ID":"<20250110154947.GE7733@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-10-pobrn@protonmail.com>\n\t\n\t\n\t<20250110105258.GA8484@pendragon.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":"","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\" "}}]