[{"id":17348,"web_url":"https://patchwork.libcamera.org/comment/17348/","msgid":"","date":"2021-05-30T16:56:13","subject":"Re: [libcamera-devel] [PATCH v1] Fix todo in\n\tgst_libcamera_src_task_enter()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nThank you for the patch.\n\nI'll let Nicolas comment on the change itself, here's a comment on the\nsubject in the meantime.\n\nThe subjlect line should describe the effects of the change, so that\nsomeone reading it without looking at the rest of the commit message\ncould get an idea of what is being done. Also, please look at the\nhistory of the file (and the directory) to see how to format the subject\n(in this case, it should start with a 'gst:' prefix).\n\nOn Sat, May 29, 2021 at 02:30:11AM +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\n> generated camera config are equal to the roles passed to it. But since,\n> if generateConfiguration() is passed a invalid role, it returns a\n> nullptr. Added error checking so that it checks the return value, and\n> sends an error message on GstBus.\n\nThe commit message itself should tell the reason for the change. You're\nnearly there, you're describing the change itself, but you don't tell\nwhy this is a good idea.\n\n> Signed-off-by: Vedant Paranjape \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 87246b40..8b6057df 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -375,11 +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> -\tg_assert(state->config_->size() == state->srcpads_.size());\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> \n> \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> \t\tGstPad *srcpad = state->srcpads_[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 48A86C3206\n\tfor ;\n\tSun, 30 May 2021 16:56:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9727E6891E;\n\tSun, 30 May 2021 18:56:25 +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 7553D6891D\n\tfor ;\n\tSun, 30 May 2021 18:56:23 +0200 (CEST)","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 E980350E;\n\tSun, 30 May 2021 18:56:22 +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=\"ZGwOjXbA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622393783;\n\tbh=RqPY6t8dObBcr2mQcE5OFlhcX9ekbG2naJL+l5On3DE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZGwOjXbAYcTR8P4eILMVZYsFlla+QiiLbXFhVyAp6hNYK52/K2lrzjHD0/vVFcIfW\n\tmbI1GqbPkohWX/TcNkXAAH/KlbNmb7XQQ7n0FepSf2Jie1ZK7SPcfFRIVjlCqJRbW6\n\t+NoSVsZugioE1J6ivRKdOlen0j+esQcNCCmUnM3c=","Date":"Sun, 30 May 2021 19:56:13 +0300","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20210528210011.154346-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210528210011.154346-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v1] Fix todo 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":17357,"web_url":"https://patchwork.libcamera.org/comment/17357/","msgid":"<75d38b5bb142f2c7d454d604c3932f60f5d95c2a.camel@ndufresne.ca>","date":"2021-05-31T14:02:46","subject":"Re: [libcamera-devel] [PATCH v1] Fix todo 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 dimanche 30 mai 2021 à 19:56 +0300, Laurent Pinchart a écrit :\n> Hi Vedant,\n> \n> Thank you for the patch.\n> \n> I'll let Nicolas comment on the change itself, here's a comment on the\n> subject in the meantime.\n> \n> The subjlect line should describe the effects of the change, so that\n> someone reading it without looking at the rest of the commit message\n> could get an idea of what is being done. Also, please look at the\n> history of the file (and the directory) to see how to format the subject\n> (in this case, it should start with a 'gst:' prefix).\n> \n> On Sat, May 29, 2021 at 02:30:11AM +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\n> > generated camera config are equal to the roles passed to it. But since,\n> > if generateConfiguration() is passed a invalid role, it returns a\n> > nullptr. Added error checking so that it checks the return value, and\n> > sends an error message on GstBus.\n> \n> The commit message itself should tell the reason for the change. You're\n> nearly there, you're describing the change itself, but you don't tell\n> why this is a good idea.\n> \n> > Signed-off-by: Vedant Paranjape \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 12 +++++++-----\n> > 1 file changed, 7 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 87246b40..8b6057df 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -375,11 +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> > -\tg_assert(state->config_->size() == state->srcpads_.size());\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\nThis change seems correct in regard to today's behaviour of libcamera.\n\n> > \n> > \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n\nI'm just a little worried that we dropped the guard for overrun. We have the\nnumber of pads that match the number of roles, I understood that\ngenerateConfiguration() will fail (return nullptr) if it cannot return the same\nnumber of stream. Though, if that rule change in the future, we would have no\nguard and would fail or crash a lot deeper in the code.\n\nPerhaps keep that original assertion ?\n\n> > \t\tGstPad *srcpad = state->srcpads_[i];\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 A4212C3206\n\tfor ;\n\tMon, 31 May 2021 14:02:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD91D68921;\n\tMon, 31 May 2021 16:02:50 +0200 (CEST)","from mail-qt1-x832.google.com (mail-qt1-x832.google.com\n\t[IPv6:2607:f8b0:4864:20::832])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C086602A8\n\tfor ;\n\tMon, 31 May 2021 16:02:49 +0200 (CEST)","by mail-qt1-x832.google.com with SMTP id c10so7949905qtx.10\n\tfor ;\n\tMon, 31 May 2021 07:02:49 -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\tc20sm8609427qtm.52.2021.05.31.07.02.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 31 May 2021 07:02:47 -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=\"dPCotRbo\"; 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:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=EbHwdZkXT2Hx+mj1pTvxEt3cj/CkI6u+0W8/mDDyuIg=;\n\tb=dPCotRboXhxYvK3D6lMKTgg6IYbCI8QsPa/Yg0bz9DmUkfIgrJZy6Isqad3jLuANzo\n\tcpDWFdRh/D9m7dbR4sr7fCiAqI9aluQoGEijFnxuJEldtdklF+A+BOdOdpeR/8pwIIDX\n\trMDobvjLMQm9ZAS2pGnJB+d8mlkaARU0tyxcHJmj2vyHjkXOrqj27V7lIiaMoraP72aP\n\tOjRCuzfuGICRUswcnCTVRlgLy6+p7hAsvX+z0ldRiqd/LumVquflJnbB3RJ/bnLnI0xL\n\tl27k6ZQApCT/X+XHrThm0hqBUFGnOTrKOrPnVMwOjznN0dIT/ozgGndeGXyPT6TjucNx\n\tq8eg==","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:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=EbHwdZkXT2Hx+mj1pTvxEt3cj/CkI6u+0W8/mDDyuIg=;\n\tb=UpWD3zekZhXYASjQs4OQEsVd8HPK5VjUT67wJpoHNWygJd81Mk6scR5r0f7NA5Dr2T\n\tv1V78PHk8JC7DYsic7pQf3jQ78fHySltfUtNBNrDG/7HIuZt2IQv+jchv7/wbrSLzdhV\n\t0fh9m7OQe4eX1jBmcOWo9qoEZvIKJCNlr9FDpGORotehXHXnGfhMtr8nPI3CrG55oOA6\n\tCdWSevkVZQXqGP5Q98C4BQ6mC9IABM7D9D/TXlyFVhtxdtn5jgokyIvxVZw8wU6yD7hJ\n\tKTHgTkMiOrSti5PoEO/VKSVZtiPhRuLsNbnNssg4wydG9xo3qwe7jPl6SvzqEYsFdmUy\n\tfaVQ==","X-Gm-Message-State":"AOAM531oQIkHwKINeCPDGxDOoPB9HbLPk8Qon5wvBlykI/4hV0Oafkf5\n\tLzZG7ZLvTxMYKm8xTsvSJ8Ie0Q==","X-Google-Smtp-Source":"ABdhPJxl/TsbHypCvoc1OX6yd2wO+eZQbTr6j4LrefgXhIimhbNn9zVTBl+gtHM4KPzdFCD6kovVWw==","X-Received":"by 2002:ac8:6908:: with SMTP id\n\te8mr15085673qtr.174.1622469767830; \n\tMon, 31 May 2021 07:02:47 -0700 (PDT)","Message-ID":"<75d38b5bb142f2c7d454d604c3932f60f5d95c2a.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Vedant Paranjape\n\t","Date":"Mon, 31 May 2021 10:02:46 -0400","In-Reply-To":"","References":"<20210528210011.154346-1-vedantparanjape160201@gmail.com>\n\t","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 v1] Fix todo 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\" "}}]