[{"id":17367,"web_url":"https://patchwork.libcamera.org/comment/17367/","msgid":"<25404c040c422a34d2dee46b03a91856bf3ef4ce.camel@ndufresne.ca>","date":"2021-06-01T21:08:31","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 02 juin 2021 à 02:26 +0530, Vedant Paranjape a écrit :\n> Error checking the output from generateConfiguration() was missing in\n> the code. Only assert was added as a guard which checked if the size of\n\nPerhaps a little rephrase would help ?\n\n> generated camera config was equal to size of roles passed to it.\n> \n> If the roles argument has a invalid member, it will return a nullptr and then\n> trying to access a member on a nullptr for size comparison will result in a\n> segmentation fault. So, if the function returns a nullptr, it will simply push\n> an error message on GstBus and gracefully exit.\n> \n> Signed-off-by: Vedant Paranjape \n\nWith better text, you have my:\n\nReviewed-by: Nicolas Dufresne \n\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 87246b40..ccc61590 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \n> \t/* Generate the stream configurations, there should be one per pad. */\n> \tstate->config_ = state->cam_->generateConfiguration(roles);\n> -\t/*\n> -\t * \\todo Check if camera may increase or decrease the number of streams\n> -\t * regardless of the number of roles.\n> -\t */\n> +\tif (state->config_ == nullptr) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t (\"Failed to generate camera configuration from roles\"),\n> +\t\t\t\t (\"Camera::generateConfiguration() returned nullptr\"));\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n> +\t}\n> \tg_assert(state->config_->size() == state->srcpads_.size());\n> \n> \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {","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 A5386C3205\n\tfor ;\n\tTue, 1 Jun 2021 21:08:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C09068920;\n\tTue, 1 Jun 2021 23:08:52 +0200 (CEST)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 770456891F\n\tfor ;\n\tTue, 1 Jun 2021 23:08:50 +0200 (CEST)","by mail-qk1-x72a.google.com with SMTP id c20so286205qkm.3\n\tfor ;\n\tTue, 01 Jun 2021 14:08:50 -0700 (PDT)","from nicolas-tpx395.localdomain (173-246-12-168.qc.cable.ebox.net.\n\t[173.246.12.168]) by smtp.gmail.com with ESMTPSA id\n\tp19sm11886406qki.119.2021.06.01.14.08.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 01 Jun 2021 14:08:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20150623.gappssmtp.com\n\theader.i=@ndufresne-ca.20150623.gappssmtp.com\n\theader.b=\"GjCX44iu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:date:in-reply-to:references:user-agent\n\t:mime-version:content-transfer-encoding;\n\tbh=FNeQKY6lQb7FzOepNH+CI3QPFN70cFlJlLyOTbxVLCU=;\n\tb=GjCX44iuCoI5vvrWuwQFe5jrGl/6h/pse3rsL7GRi1jHtzlxYYYK/tC4p3SflAlCin\n\tKzrDHCO/M2w4XCbebsO6Jgw2nGFkoM1LBg3kcxSKwMf5hUoGaJOmI0HG16fBkH8N5Cgs\n\tUbq2I+z2/QLhRfri1CWUWVLDhqfJOjxnLtSacR4jwNmbGdNqfIBpQIsWlNwLjaVpzJZw\n\tkQuIGIsrWMR2ynItq7TTm/nJOBqxdUJxyhxWWBpn9c5MWaL8qgMZKATuHpm1dWNGxQc9\n\tJp7sOVwBfLKAr5J42Km4ua+3svfctIL15TEYxEh57XxIqGVkgsNNuLL4ma4bQYS8J4Jl\n\tgukQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=FNeQKY6lQb7FzOepNH+CI3QPFN70cFlJlLyOTbxVLCU=;\n\tb=trdapvW96cBy/qMQnwaqBgA1+WT21gCNxToY++RGv6lFRx18ylO0l7mVqhK/ofx870\n\thoLM0/KHq10sNmiJmBIZAjpFXI9FwqT+VofB1XLU8a+dKuYxULDWEBX0/EefgUKo9iaP\n\tHUO3Jpc3l05afacjmHNvWbUtRMIaFdpKBf8UarnyC7r7V9oF6V6fb7qHBOtxmGMVwrKW\n\t/fiKtA/g8kfUEarEpZFuPSAy6o3WoyKPqxiVwLmEGmlPMaYnQ3QT0Rh/xITWhOyoq84w\n\trsbeQMI/g/NzPyACyHzpYMqmSfVCIdyfEGwtaHBlgcUBAhWOQLntdUpBCndpiYqAoah+\n\tFS9A==","X-Gm-Message-State":"AOAM531VYrWd+4ESiLkQY983iYw7orcxNr7KhU/gxwWxM7vA8FrPV9gq\n\tIbIaOR21epkWb6CeGRD98Bp67A==","X-Google-Smtp-Source":"ABdhPJxDS+OWLchpMkR3nhMIQG4X7a3aDuUoMu8TGjtN0a++ist6i3M0BFmy9ptZxCQMJt25Ct/BSQ==","X-Received":"by 2002:a37:c0a:: with SMTP id 10mr22982680qkm.194.1622581729093;\n\tTue, 01 Jun 2021 14:08:49 -0700 (PDT)","Message-ID":"<25404c040c422a34d2dee46b03a91856bf3ef4ce.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Vedant Paranjape , \n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 01 Jun 2021 17:08:31 -0400","In-Reply-To":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>","References":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.40.1 (3.40.1-1.fc34) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","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":17379,"web_url":"https://patchwork.libcamera.org/comment/17379/","msgid":"<20210602094528.GH1929@pyrite.rasen.tech>","date":"2021-06-02T09:45:28","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Vedant,\n\nOn Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:\n> Error checking the output from generateConfiguration() was missing in\n> the code. Only assert was added as a guard which checked if the size of\n> generated camera config was equal to size of roles passed to it.\n\nMaybe \"The return value from generateConfiguration() was not checked\" ?\n\n> \n> If the roles argument has a invalid member, it will return a nullptr and then\n\nIt's not really about roles having an invalid member, it's about the\ncamera not supporting a requested role.\n\n> trying to access a member on a nullptr for size comparison will result in a\n> segmentation fault. So, if the function returns a nullptr, it will simply push\n> an error message on GstBus and gracefully exit.\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 87246b40..ccc61590 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \n> \t/* Generate the stream configurations, there should be one per pad. */\n> \tstate->config_ = state->cam_->generateConfiguration(roles);\n> -\t/*\n> -\t * \\todo Check if camera may increase or decrease the number of streams\n> -\t * regardless of the number of roles.\n> -\t */\n> +\tif (state->config_ == nullptr) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t (\"Failed to generate camera configuration from roles\"),\n> +\t\t\t\t (\"Camera::generateConfiguration() returned nullptr\"));\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n> +\t}\n> \tg_assert(state->config_->size() == state->srcpads_.size());\n\nI thought the whole point of adding the error checking is to remove this\nassert?\n\nIf the return value is not nullptr, then the number of\nStreamConfigurations in the CameraConfiguration is guaranteed to be the\nsame as the number of roles that were requested. If any role is not\nsupported, then nullptr will be returned. So this assertion, if reached,\nwill always be true.\n\n\nPaul\n\n> \n> \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> -- \n> 2.25.1\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 83249C3208\n\tfor ;\n\tWed, 2 Jun 2021 09:45:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF47668927;\n\tWed, 2 Jun 2021 11:45:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5B89602A7\n\tfor ;\n\tWed, 2 Jun 2021 11:45:36 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 694CBB2C;\n\tWed, 2 Jun 2021 11:45:35 +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=\"XIhjd9E+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622627136;\n\tbh=lEh3zPlg5ZCM9HvP+H+GXqzcmYYvdyvcIKCnuQ50ly0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XIhjd9E+FhmM6chupVdm7zj5s1XEY1KOw6XRYncpfWfdmbTUG4A12cqSEkBodBdeX\n\t9iK0zJ3cb1RhQsCHHjqEvijWDsSQOyg7UP8sZo/qDFYPw2aa8Bx+cWd+ILhTG2w3iw\n\t/uLMm5ws73hHkJlAn+DrE24J04FIQqEP+awWUO9c=","Date":"Wed, 2 Jun 2021 18:45:28 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210602094528.GH1929@pyrite.rasen.tech>","References":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","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":17386,"web_url":"https://patchwork.libcamera.org/comment/17386/","msgid":"","date":"2021-06-02T11:36:48","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hi Paul\n\nOn Wed, Jun 2, 2021 at 3:15 PM wrote:\n\n> Hi Vedant,\n>\n> On Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:\n> > Error checking the output from generateConfiguration() was missing in\n> > the code. Only assert was added as a guard which checked if the size of\n> > generated camera config was equal to size of roles passed to it.\n>\n> Maybe \"The return value from generateConfiguration() was not checked\" ?\n>\n\nSounds much better, will make the change.\n\n\n> >\n> > If the roles argument has a invalid member, it will return a nullptr and\n> then\n>\n> It's not really about roles having an invalid member, it's about the\n> camera not supporting a requested role.\n>\n\nWell, I could add a random number to StreamRoles vector, but I get your\npoint, I'll make it invalid/unsupported role.\n\n\n> > trying to access a member on a nullptr for size comparison will result\n> in a\n> > segmentation fault. So, if the function returns a nullptr, it will\n> simply push\n> > an error message on GstBus and gracefully exit.\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----\n> > 1 file changed, 7 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 87246b40..ccc61590 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >\n> > /* Generate the stream configurations, there should be one per\n> pad. */\n> > state->config_ = state->cam_->generateConfiguration(roles);\n> > - /*\n> > - * \\todo Check if camera may increase or decrease the number of\n> streams\n> > - * regardless of the number of roles.\n> > - */\n> > + if (state->config_ == nullptr) {\n> > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > + (\"Failed to generate camera\n> configuration from roles\"),\n> > + (\"Camera::generateConfiguration()\n> returned nullptr\"));\n> > + gst_task_stop(task);\n> > + return;\n> > + }\n> > g_assert(state->config_->size() == state->srcpads_.size());\n>\n> I thought the whole point of adding the error checking is to remove this\n> assert?\n>\n> If the return value is not nullptr, then the number of\n> StreamConfigurations in the CameraConfiguration is guaranteed to be the\n> same as the number of roles that were requested. If any role is not\n> supported, then nullptr will be returned. So this assertion, if reached,\n> will always be true.\n>\n>\nI agree with this part, but Nicolas insisted on keeping it there. In case\nsomething changes in the future.\nAlso, it asserts if the following expression turns false. You mean this? if\nI understood it rightly.\nhttps://developer.gnome.org/glib/stable/glib-Testing.html#g-assert\n\nRegards\n*Vedant Paranjape*","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 EFD45C3208\n\tfor ;\n\tWed, 2 Jun 2021 11:37:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 729E56050E;\n\tWed, 2 Jun 2021 13:37:02 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C55C9602A4\n\tfor ;\n\tWed, 2 Jun 2021 13:37:00 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id y2so3311969ybq.13\n\tfor ;\n\tWed, 02 Jun 2021 04:37:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"IQab/6UM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tIUeRb/55Kq9RQkcz06fD/xUY1cDaEwAP9A4pAKqKFs=;\n\tb=IQab/6UMMxYjDVXM9p/Fe6fwtqqa556c1J8aWMs5pN2kYw6FPqVG/CGpnAiJAdHd8a\n\tYVWM5+LWtpnh/BTcQTNuFFBsb+5toBQnE/YIQQXnLvbK37c3gpAo6JYBZOclI2Fwda34\n\t0mcnCXHsVQZD267WDvTTRXk+OpFOlhlq/iBIwx+81Oe8d9xYMwTimgBgbsh1tk06DoPa\n\tBOOCSaa5LrQtbeXKKoOyJ7/vlPr4lNT+jhW+pfIDA57w0/bwvymBpgiwVeb7dKYxlyxn\n\ttJOyLWVpTrdPGHXbFbHmNPmI/TthJL4WUPiz1RIoZObZFtzvKUZj0ezpe/I6XZI5m0Ek\n\t1zaw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tIUeRb/55Kq9RQkcz06fD/xUY1cDaEwAP9A4pAKqKFs=;\n\tb=Br10mv48OqVsTzlDXJDdPHR3k7Cp7iEXtHST+VirIZN2eVAMYLJ0w/uxRJ87VsKZX6\n\tpFSnMT8mWhiN3E1XQMhrT+fi2FBTOOGBPrj9c9f/oHVnRE7HFxArruNiRBOD8JxLzVPT\n\tKNd/w9edfUd388P5cyviMFzpK56G1g22He/bXq3QG4lE7DeEQzttqc39ZUBvJ7nZ3oAk\n\to3Sc8e8l6GEV+nZ3LB7qyUbxMLdAVmnINEAZBsAs8sCCf4q6Y/xJtsakAY3BLX5d1R8J\n\t2zkGYKdJsfD9iDDXuQLMrp5WH9ns2OKRzlPtsuR8RAK+sA05DzW2rQYs5SxaEvp5Dgy+\n\tG25w==","X-Gm-Message-State":"AOAM532PrqOTDfXZ+dEfwav25JUBJmzepFa5BR29WN0hZYw1zlt+NX4U\n\ts/UjJAu/g0TCwDtGRnBOPDNXdcyBZMLM92cik4w=","X-Google-Smtp-Source":"ABdhPJwkKRDXZM07LB6VfETNCQofQ3IfyjIE5el8/YDd+FTX1qKzYbsxZaz3RJuOTqIqelUkmktc3/8RaK3j1r54SIg=","X-Received":"by 2002:a25:6c08:: with SMTP id\n\th8mr46306781ybc.175.1622633819497; \n\tWed, 02 Jun 2021 04:36:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>\n\t<20210602094528.GH1929@pyrite.rasen.tech>","In-Reply-To":"<20210602094528.GH1929@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Wed, 2 Jun 2021 17:06:48 +0530","Message-ID":"","To":"paul.elder@ideasonboard.com","Content-Type":"multipart/alternative; boundary=\"0000000000006969b605c3c6e0cb\"","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","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":17393,"web_url":"https://patchwork.libcamera.org/comment/17393/","msgid":"<20210603051027.GB156622@pyrite.rasen.tech>","date":"2021-06-03T05:10:27","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jun 02, 2021 at 05:06:48PM +0530, Vedant Paranjape wrote:\n> Hi Paul\n> \n> On Wed, Jun 2, 2021 at 3:15 PM wrote:\n> \n> Hi Vedant,\n> \n> On Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:\n> > Error checking the output from generateConfiguration() was missing in\n> > the code. Only assert was added as a guard which checked if the size of\n> > generated camera config was equal to size of roles passed to it.\n> \n> Maybe \"The return value from generateConfiguration() was not checked\" ?\n> \n> \n> Sounds much better, will make the change.\n>  \n> \n> >\n> > If the roles argument has a invalid member, it will return a nullptr and\n> then\n> \n> It's not really about roles having an invalid member, it's about the\n> camera not supporting a requested role.\n> \n> \n> Well, I could add a random number to StreamRoles vector, but I get your point,\n\nIn theory, yes. In practice, no:\n\n../../src/gstreamer/gstlibcamerasrc.cpp:376:18: error: invalid\nconversion from ‘int’ to\n‘std::vector::value_type’ {aka\n‘libcamera::StreamRole’} [-fpermissive]\n\nThat's what we have types and compiler errors for :D\n\n(unless you static_cast it)\n\n> I'll make it invalid/unsupported role.\n\nBut yeah, that's fine.\n\n>  \n> \n> > trying to access a member on a nullptr for size comparison will result in\n> a\n> > segmentation fault. So, if the function returns a nullptr, it will simply\n> push\n> > an error message on GstBus and gracefully exit.\n> >\n> > Signed-off-by: Vedant Paranjape \n> > ---\n> >  src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----\n> >  1 file changed, 7 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> gstlibcamerasrc.cpp\n> > index 87246b40..ccc61590 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> > \n> >       /* Generate the stream configurations, there should be one per pad.\n> */\n> >       state->config_ = state->cam_->generateConfiguration(roles);\n> > -     /*\n> > -      * \\todo Check if camera may increase or decrease the number of\n> streams\n> > -      * regardless of the number of roles.\n> > -      */\n> > +     if (state->config_ == nullptr) {\n> > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +                               (\"Failed to generate camera configuration\n> from roles\"),\n> > +                               (\"Camera::generateConfiguration()\n> returned nullptr\"));\n> > +             gst_task_stop(task);\n> > +             return;\n> > +     }\n> >       g_assert(state->config_->size() == state->srcpads_.size());\n> \n> I thought the whole point of adding the error checking is to remove this\n> assert?\n> \n> If the return value is not nullptr, then the number of\n> StreamConfigurations in the CameraConfiguration is guaranteed to be the\n> same as the number of roles that were requested. If any role is not\n> supported, then nullptr will be returned. So this assertion, if reached,\n> will always be true.\n> \n> \n>  \n> I agree with this part, but Nicolas insisted on keeping it there. In case\n> something changes in the future.\n\nAh okay. I guess it's fine to keep then.\n\n> Also, it asserts if the following expression turns false. You mean this? if I\n> understood it rightly.\n> https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert\n\nassert() asserts that the expression is true. If it is not true, then it\nterminates. So assert makes sure that the expression is always true.\n\n\nPaul","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 4927AC3208\n\tfor ;\n\tThu, 3 Jun 2021 05:10:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE93A68923;\n\tThu, 3 Jun 2021 07:10:36 +0200 (CEST)","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 A8095602A2\n\tfor ;\n\tThu, 3 Jun 2021 07:10:35 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 107C6EF;\n\tThu, 3 Jun 2021 07:10:33 +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=\"asz5ZYPb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622697035;\n\tbh=Q9I3Y799g6oVpQPKNOf4Htrtl4QxARWv+s+rTRtex3E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=asz5ZYPbDk1KMPqsf2p1CmHe/pPgRD10ps5D+/LIdE7jqFOuztqzlsTeGcRoE6Q78\n\tuPt1UZOWoupSxFIkIk/KN0WQwyzTTtuG1U41YrkHEpcctLBAEuVDwDsy4Y2gvcJ2H+\n\t/V31fqfX7Fgu1P4XWjJ2h1Vq7c8NRJ/5F/9wKrYs=","Date":"Thu, 3 Jun 2021 14:10:27 +0900","From":"paul.elder@ideasonboard.com","To":"Vedant Paranjape ","Message-ID":"<20210603051027.GB156622@pyrite.rasen.tech>","References":"<20210601205640.14334-1-vedantparanjape160201@gmail.com>\n\t<20210602094528.GH1929@pyrite.rasen.tech>\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 v3] gstreamer: Add error checking in\n\tgst_libcamera_src_task_enter()","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\" "}}]