[{"id":35295,"web_url":"https://patchwork.libcamera.org/comment/35295/","msgid":"","date":"2025-08-07T12:55:39","subject":"Re: [PATCH] gstreamer: Drop a redundant conditional check","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi,\n\nLe jeudi 07 août 2025 à 12:51 +0530, Umang Jain a écrit :\n> RequestWrap `wrap` is set only if completedRequests_ queue\n> is not empty, and if the `wrap` remains unset, -ENOBUFS is returned\n> by GstLibcameraSrcState::processRequest().\n> \n> There is no need to set the `err` variable to -ENOBUFS,\n> by checking the completedRequests_ queue again. Hence, drop this\n> conditional check from the processRequest() hotpath.\n> \n> Signed-off-by: Umang Jain \n\nI agree with the removal of the check, so for that:\n\nReviewed-by: Nicolas Dufresne \n\nThough, the commit message describe this as if there is no behaviour change. In\npractice, there is a behaviour change. Before the change, if the request was the\nlast of the queue, it would process that request and return -ENOBUFS. While now,\nit will request that request and return 0.\n\nThis change the behaviour of the switch:\n\n\tret = state->processRequest();\n\tswitch (ret) {\n\tcase 0:\n\t\t/* Another completed request is available, resume the task. */\n\t\tdoResume = true;\n\t\tbreak;\n\n\tcase -EPIPE:\n\t\tgst_task_stop(self->task);\n\t\treturn;\n\n\tcase -ENOBUFS:\n\tdefault:\n\t\tbreak;\n\n\nWith the effect that once it popped the last request, it will try once again\nbefore pausing the task. This task dynamic is known racy, \n\nhttps://bugs.libcamera.org/show_bug.cgi?id=201\n\nI don't think this removal will completely fix the race, but it will likely\nreduce it. Can you track down the reproducer from bug 201 and see what\nimplication it has ? Then please rewrite your commit message to properly\ndescribe the effect of your changes.\n\nNicolas\n\n> ---\n>  src/gstreamer/gstlibcamerasrc.cpp | 3 ---\n>  1 file changed, 3 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 79a025a5..3adb34f9 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -346,9 +346,6 @@ int GstLibcameraSrcState::processRequest()\n>  \t\t\twrap = std::move(completedRequests_.front());\n>  \t\t\tcompletedRequests_.pop();\n>  \t\t}\n> -\n> -\t\tif (completedRequests_.empty())\n> -\t\t\terr = -ENOBUFS;\n>  \t}\n>  \n>  \tif (!wrap)","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 9732FBDCC1\n\tfor ;\n\tThu, 7 Aug 2025 12:55:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 802D66921A;\n\tThu, 7 Aug 2025 14:55:43 +0200 (CEST)","from bali.collaboradmins.com (bali.collaboradmins.com\n\t[148.251.105.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50A7A69052\n\tfor ;\n\tThu, 7 Aug 2025 14:55:42 +0200 (CEST)","from [IPv6:2606:6d00:11:5a76::5ac] (unknown\n\t[IPv6:2606:6d00:11:5a76::5ac])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bali.collaboradmins.com (Postfix) with ESMTPSA id 478E017E0509;\n\tThu, 7 Aug 2025 14:55:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=collabora.com header.i=@collabora.com\n\theader.b=\"dtiVMu4m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1754571341;\n\tbh=84QSqYi1gcKcLCzKdFQKBdtaHGq1heAUMrYHxGu5io0=;\n\th=Subject:From:To:Date:In-Reply-To:References:From;\n\tb=dtiVMu4mgxs47cauNKSyLFIxDHGSc7wQW/r5jylOWgeZoiZKPvtFMvNKnVM0T0zZk\n\t0h6Kb6RXiEEDWfYK0FMeTRt+RtBLbJCHag5EXOF0YCJne965uTIgRmvYLDWCRsSxmY\n\tazjtgxxUw24rBXbTJyldRmcgFp9dDN2aRPZPLERe/5iQLQocHEZOUNrQmCVu5Aw0Kq\n\tIefmQ3Hdc6PIpVyUtkeofpGfyi9e7py6rhZnQM9itxi/7zUWvVIRSF9GblwK2qZbI4\n\t8bVj305eNJPYHjNDbyolZyTpJaqFS60UBt50RNhtrP6zvfj0QQXaOz0/BVaQumjxSZ\n\tgg2zhYSsplnDQ==","Message-ID":"","Subject":"Re: [PATCH] gstreamer: Drop a redundant conditional check","From":"Nicolas Dufresne ","To":"Umang Jain , libcamera-devel@lists.libcamera.org","Date":"Thu, 07 Aug 2025 08:55:39 -0400","In-Reply-To":"<20250807072123.35523-1-uajain@igalia.com>","References":"<20250807072123.35523-1-uajain@igalia.com>","Autocrypt":"addr=nicolas.dufresne@collabora.com; prefer-encrypt=mutual;\n\tkeydata=mQGiBEUQN0MRBACQYceNSezSdMjx7sx6gwKkMghrrODgl3B0eXBTgNp6c431IfOOEsdvk\n\toOh1kwoYcQgbg4MXw6beOltysX4e8fFWsiRkc2nvvRW9ir9kHDm49MkBLqaDjTqOkYKNMiurFW+go\n\tzpr/lUW15QqT6v68RYe0zRdtwGZqeLzX2LVuukGwCg4AISzswrrYHNV7vQLcbaUhPgIl0D+gILYT9\n\tTJgAEK4YHW+bFRcY+cgUFoLQqQayECMlctKoLOE69nIYOc/hDr9uih1wxrQ/yL0NJvQCohSPyoyLF\n\t9b2EuIGhQVp05XP7FzlTxhYvGO/DtO08ec85+bTfVBMV6eeY4MS3ZU+1z7ObD7Pf29YjyTehN2Dan\n\t6w1g2rBk5MoA/9nDocSlk4pbFpsYSFmVHsDiAOFje3+iY4ftVDKunKYWMhwRVBjAREOByBagmRau0\n\tcLEcElpf4hX5f978GoxSGIsiKoDAlXX+ICDOWC1/EXhEEmBR1gL0QJgiVviNyLfGJlZWnPjw6xhhm\n\ttHYWTDxBOP5peztyc2PqeKsLsLWzAr7QnTmljb2xhcyBEdWZyZXNuZSA8bmljb2xhc0BuZHVmcmVz\n\tbmUuY2E+iGIEExECACIFAlXA3CACGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJEHFTAi2sB\n\tqgcJngAnRDBTr8bhzuH0KQwFP1nEYtfgpKdAKCrQ/sJfuG/8zsd7J8wVl7y3e8ARbRDTmljb2xhcy\n\tBEdWZyZXNuZSAoQi4gU2MuIEluZm9ybWF0aXF1ZSkgPG5pY29sYXMuZHVmcmVzbmVAZ21haWwuY29\n\ttPohgBBMRAgAgBQJFlCyOAhsDBgsJCAcDAgQVAggDBBYCAwECHgECF4AACgkQcVMCLawGqBwhLQCg\n\tzYlrLBj6KIAZ4gmsfjXD6ZtddT8AoIeGDicVq5WvMHNWign6ApQcZUihtElOaWNvbGFzIER1ZnJlc\n\t25lIChCLiBTYy4gSW5mb3JtYXRpcXVlKSA8bmljb2xhcy5kdWZyZXNuZUBjb2xsYWJvcmEuY28udW\n\ts+iGIEExECACIFAkuzca8CGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJEHFTAi2sBqgcQX8\n\tAn2By6LDEeMxi4B9hUbpvRnzaaeNqAJ9Rox8rfqHZnSErw9bCHiBwvwJZ77QxTmljb2xhcyBEdWZy\n\tZXNuZSA8bmljb2xhcy5kdWZyZXNuZUBjb2xsYWJvcmEuY29tPohiBBMRAgAiBQJNzZzPAhsDBgsJC\n\tAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRBxUwItrAaoHLlxAKCYAGf4JL7DYDLs/188CPMGuwLypw\n\tCfWKc9DorA9f5pyYlD5pQo6SgSoiC0R05pY29sYXMgRHVmcmVzbmUgKEIgU2MuIEluZm9ybWF0aXF\n\t1ZSkgPG5pY29sYXMuZHVmcmVzbmVAdXNoZXJicm9va2UuY2E+iGAEExECACAFAkUQN0MCGwMGCwkI\n\tBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRBxUwItrAaoHPTnAJ0WGgJJVspoctAvEcI00mtp5WAFGgCgr\n\t+E7ItOqZEHAs+xabBgknYZIFPU=","Organization":"Collabora Canada","Content-Type":"multipart/signed; micalg=\"pgp-sha1\";\n\tprotocol=\"application/pgp-signature\"; \n\tboundary=\"=-D45RSX0Y1t2rzJ+55e9j\"","User-Agent":"Evolution 3.56.2 (3.56.2-1.fc42) ","MIME-Version":"1.0","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":35299,"web_url":"https://patchwork.libcamera.org/comment/35299/","msgid":"","date":"2025-08-07T14:03:34","subject":"Re: [PATCH] gstreamer: Drop a redundant conditional check","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"Hi Nicolas\n\nOn 2025-08-07 18:25, Nicolas Dufresne wrote:\n> Hi,\n> \n> Le jeudi 07 août 2025 à 12:51 +0530, Umang Jain a écrit :\n>> RequestWrap `wrap` is set only if completedRequests_ queue\n>> is not empty, and if the `wrap` remains unset, -ENOBUFS is returned\n>> by GstLibcameraSrcState::processRequest().\n>> \n>> There is no need to set the `err` variable to -ENOBUFS,\n>> by checking the completedRequests_ queue again. Hence, drop this\n>> conditional check from the processRequest() hotpath.\n>> \n>> Signed-off-by: Umang Jain \n> \n> I agree with the removal of the check, so for that:\n> \n> Reviewed-by: Nicolas Dufresne \n> \n> Though, the commit message describe this as if there is no behaviour change. In\n> practice, there is a behaviour change. Before the change, if the request was the\n> last of the queue, it would process that request and return -ENOBUFS. While now,\n> it will request that request and return 0.\n\nYes, I am aware of that behaviour and I agree I should have done better\njob in the commit message. Removal the check will lead to one more\niteration to pause the GstTask, but eventually\nit will be paused with -ENOBUFS, so from that pov, I implied that no\nbehaviorable change as such.\n\n> \n> This change the behaviour of the switch:\n> \n> \tret = state->processRequest();\n> \tswitch (ret) {\n> \tcase 0:\n> \t\t/* Another completed request is available, resume the task. */\n> \t\tdoResume = true;\n> \t\tbreak;\n> \n> \tcase -EPIPE:\n> \t\tgst_task_stop(self->task);\n> \t\treturn;\n> \n> \tcase -ENOBUFS:\n> \tdefault:\n> \t\tbreak;\n> \n> \n> With the effect that once it popped the last request, it will try once again\n> before pausing the task. This task dynamic is known racy, \n> \n> https://bugs.libcamera.org/show_bug.cgi?id=201\n\nOh, this was not on my radar. I will try to a look there.\n\n> \n> I don't think this removal will completely fix the race, but it will likely\n> reduce it. Can you track down the reproducer from bug 201 and see what\n> implication it has ? Then please rewrite your commit message to properly\n> describe the effect of your changes.\n\nAck.\n> \n> Nicolas\n> \n>> ---\n>>  src/gstreamer/gstlibcamerasrc.cpp | 3 ---\n>>  1 file changed, 3 deletions(-)\n>> \n>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>> index 79a025a5..3adb34f9 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> @@ -346,9 +346,6 @@ int GstLibcameraSrcState::processRequest()\n>>  \t\t\twrap = std::move(completedRequests_.front());\n>>  \t\t\tcompletedRequests_.pop();\n>>  \t\t}\n>> -\n>> -\t\tif (completedRequests_.empty())\n>> -\t\t\terr = -ENOBUFS;\n>>  \t}\n>>  \n>>  \tif (!wrap)","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 E14F5BDCC1\n\tfor ;\n\tThu, 7 Aug 2025 14:03:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1FA06921A;\n\tThu, 7 Aug 2025 16:03:39 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0240E69052\n\tfor ;\n\tThu, 7 Aug 2025 16:03:37 +0200 (CEST)","from maestria.local.igalia.com ([192.168.10.14]\n\thelo=mail.igalia.com) by fanzine2.igalia.com with esmtps \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uk1DV-00B1uL-75; Thu, 07 Aug 2025 16:03:37 +0200","from webmail.service.igalia.com ([192.168.21.45])\n\tby mail.igalia.com with esmtp (Exim)\n\tid 1uk1DS-00D7sE-Lc; Thu, 07 Aug 2025 16:03:37 +0200","from localhost ([127.0.0.1] helo=webmail.igalia.com)\n\tby webmail with esmtp (Exim 4.96) (envelope-from )\n\tid 1uk1DS-001hlX-0K; Thu, 07 Aug 2025 16:03:34 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"O5bZjJrz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=Content-Transfer-Encoding:Content-Type:Message-ID:References:\n\tIn-Reply-To:Subject:Cc:To:From:Date:MIME-Version:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=T2cwK9L2yqfdSdPcijG3d/OCflfYI8PfP8c8cL/SfrU=;\n\tb=O5bZjJrzEX1GqTZYHEAgcbsz+m\n\tAtf/347ejCAFb1t2TQznWA6kDfmH05asmc+QIaWQDu10l8/h4yHZMyBBraaIpAMnetySgpIkZ7zs3\n\tkno1BzltGeaR5OYahIi9X1Meai/d6IdsjT1AP0aS4jX6o3dx780Q2T0b4wLpaEmucwSrcXACr4wS5\n\tbRkPsQsQJyWPidSwnfzMSZncJrnk1lzymrX0U24pbs3tckN+uYK/FLAo955OIoAYu8SzLB0zIPWkv\n\teKIy034TvWl+ZSop8OM1htZtKNfc4ik4is6DiYa/Br8WpBae+nDQSPIAWWaYNQKQp07fTWiRfsahv\n\thxfqS5wA==;","MIME-Version":"1.0","Date":"Thu, 07 Aug 2025 19:33:34 +0530","From":"uajain ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] gstreamer: Drop a redundant conditional check","In-Reply-To":"","References":"<20250807072123.35523-1-uajain@igalia.com>\n\t","Message-ID":"","X-Sender":"uajain@igalia.com","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","X-Spam-Report":"NO, Score=-1.3, Tests=ALL_TRUSTED=-3, AWL=0.858, BAYES_50=0.8,\n\tURIBL_BLOCKED=0.001","X-Spam-Score":"-12","X-Spam-Bar":"-","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\" "}}]