[{"id":26471,"web_url":"https://patchwork.libcamera.org/comment/26471/","msgid":"","date":"2023-02-24T06:22:35","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Suhrid,\n\nThank you for the patch.\n\nOn 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> - If no converter is found, converter_ becomes a nullptr and\n> !converter_->isValid() causes a segmentation fault.\n> - Avoid this by checking if converter_ is a nullptr.\n>\n> Signed-off-by: Suhrid Subramaniam \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 24ded4db..2423ec10 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> \tMediaDevice *converter = pipe->converter();\n> \tif (converter) {\n> \t\tconverter_ = ConverterFactoryBase::create(converter);\n> -\t\tif (!converter_->isValid()) {\n> +\t\tif (!converter_) {\n\nit seems this change will drop the validity check so maybe :\n         if (!converter_ && converter_->isValid()) {\n\nThe error message below might be need to get tweaked a bit.\n\nOn the other hand, I wonder if ConverterFactoryBase::create() should be \nallowed? to return a non-valid converter pointer or not.\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> \t\t\tconverter_.reset();","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 D8EDABE080\n\tfor ;\n\tFri, 24 Feb 2023 06:22:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1697762660;\n\tFri, 24 Feb 2023 07:22:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DB8661EE1\n\tfor ;\n\tFri, 24 Feb 2023 07:22:41 +0100 (CET)","from [192.168.1.7] (unknown [122.172.82.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE4192E4;\n\tFri, 24 Feb 2023 07:22:39 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677219762;\n\tbh=jAp6Kxc60EuTu1uDzQpuXreUkMIHRudLkMt5xOPYop8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XchUibVDTdh0tktqpmtbH4amQQZ9CH5Jg4ig8gPoebBEEp7N2UF5jxxbaJGJ5UKPM\n\tsNEoENbFcKUcgxJ+PBX1n3H4zOTE+HrRgzQXlWNTRreBWGpl7XUym3SyIm9j0V/yAf\n\teGzQX+lh2LePcRMNiRrgE9NVxjHdLuj9E0Q/2A6ttWV6OS6Vo01LRPfJlIACm4O8gW\n\tQYaCyaN38IJV6Uos8b1PuJWHAPMlpq6zeYtNB5+eYrJa3ws2gpX3At+MaxxrawRxvP\n\t59tstGLy3HMgUtxDhumo1h5Xe4UqYndB3J6++9ptLMzKEXOCkzNIjHIzz6mo7BdSJC\n\tGEGWYcDJQc6TA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677219760;\n\tbh=jAp6Kxc60EuTu1uDzQpuXreUkMIHRudLkMt5xOPYop8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cZAqQw9Ilj0AFY+O4ktSgrtMfiMyniluSELgcFdu/ovTMwBWRVEGN6IFr6YevYAwM\n\tFIv1HmcwyrYLfL7N2OcUhlBlzXnlOMLB43YIulYdVKXQWg3nhKbLhBh6Hf3DPy4HHr\n\tmHlz6gMRh9yI7aR7Frqe8E6Rxhkq0wI/bwnGodU0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cZAqQw9I\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 24 Feb 2023 11:52:35 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Suhrid Subramaniam ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>","In-Reply-To":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"Suhrid Subramaniam ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26479,"web_url":"https://patchwork.libcamera.org/comment/26479/","msgid":"","date":"2023-02-24T20:45:59","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Umang,\n\nThanks for reviewing the patch. This is my first time using git-email so please let me know if I need to change my formatting : )\nMy comments are inline.\n\n\n-----Original Message-----\nFrom: Umang Jain \nSent: Thursday, February 23, 2023 10:23 PM\nTo: Suhrid Subramaniam ; libcamera-devel@lists.libcamera.org\nCc: Suhrid Subramaniam \nSubject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr\n\nHi Suhrid,\n\nThank you for the patch.\n\nOn 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> - If no converter is found, converter_ becomes a nullptr and\n> !converter_->isValid() causes a segmentation fault.\n> - Avoid this by checking if converter_ is a nullptr.\n>\n> Signed-off-by: Suhrid Subramaniam \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp \n> b/src/libcamera/pipeline/simple/simple.cpp\n> index 24ded4db..2423ec10 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> \tMediaDevice *converter = pipe->converter();\n> \tif (converter) {\n> \t\tconverter_ = ConverterFactoryBase::create(converter);\n> -\t\tif (!converter_->isValid()) {\n> +\t\tif (!converter_) {\n\nit seems this change will drop the validity check so maybe :\n         if (!converter_ && converter_->isValid()) {\n\nFor situations where converter_ is a nullptr, the && operation will execute converter_->isValid() and generate a segmentation fault.\nI'd originally considered updating the converter_ check to the following:\n if(!converter_ || converter_->isValid())\nLaurent said that it probably doesn't make much sense for the converter factory to return a non-null but invalid converter.\n\n\nThe error message below might be need to get tweaked a bit.\n\nYep sure. Any specific format you recommend for the error message?\n\nOn the other hand, I wonder if ConverterFactoryBase::create() should be allowed? to return a non-valid converter pointer or not.\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> \t\t\tconverter_.reset();\n\nI was thinking of some sort of exception handling/ modification to isValid but the former needs to be implemented in simple.cpp which would have the same effect as !converter_ and the latter would not work if converter_ is a nullptr. Anything you'd recommend?\n\n\nThanks!","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 97044BE080\n\tfor ;\n\tFri, 24 Feb 2023 20:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8C2E62652;\n\tFri, 24 Feb 2023 21:46:20 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677271580;\n\tbh=771v2KuqodoJOwFLSTKZ/ZFL8W9prE7hx32msR24Fbg=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=EWDs2t2Ha/uZ9SY6k+wLFDPnf24QZ4CnFwwDkWz+69lGnEma0n+g0dOl84mRhuq58\n\tGp085yDsNS0Ak30A1ZZBhn4WRdEZJtpa2SiKrzgf14rG358zork77kDmJ9KL2VfW5q\n\tJ4kbrNR86+GzmZRmjkhtmGKkroVlpSiQyOeKL8V4JKQ1J2p+A5Vucy9KDkwJRR3nP2\n\tfMX3YbczQ4A5LSpxNoj3xEHMvCS6ccDHlFoRZgrXXHckuIfHN76QsKbxkBwVEMF0Ku\n\t1gV6PC4sokk2Btp22XgN249LAYt7UL3vs2heTsfmmG3Y8JkH2AVwe8q5bnwK1SbArQ\n\t5hsMZMyHTAiuA==","To":"Umang Jain , Suhrid Subramaniam\n\t, \"libcamera-devel@lists.libcamera.org\"\n\t","Date":"Fri, 24 Feb 2023 20:45:59 +0000","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>\n\t","In-Reply-To":"","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Suhrid Subramaniam via libcamera-devel\n\t","Precedence":"list","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Suhrid Subramaniam ","List-Help":"","Subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26482,"web_url":"https://patchwork.libcamera.org/comment/26482/","msgid":"","date":"2023-02-26T15:41:36","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote:\n> Hi Suhrid,\n> \n> Thank you for the patch.\n> \n> On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> > - If no converter is found, converter_ becomes a nullptr and\n> > !converter_->isValid() causes a segmentation fault.\n> > - Avoid this by checking if converter_ is a nullptr.\n> >\n> > Signed-off-by: Suhrid Subramaniam \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 24ded4db..2423ec10 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> > \tMediaDevice *converter = pipe->converter();\n> > \tif (converter) {\n> > \t\tconverter_ = ConverterFactoryBase::create(converter);\n> > -\t\tif (!converter_->isValid()) {\n> > +\t\tif (!converter_) {\n> \n> it seems this change will drop the validity check so maybe :\n>         if (!converter_ && converter_->isValid()) {\n> \n> The error message below might be need to get tweaked a bit.\n> \n> On the other hand, I wonder if ConverterFactoryBase::create() should be \n> allowed? to return a non-valid converter pointer or not.\n\nI don't see a use case for that, so I'd rather check for !valid in\n::create() and return nullptr in that case.\n\n> > \t\t\tLOG(SimplePipeline, Warning)\n> > \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> > \t\t\tconverter_.reset();","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 8AF8DBE080\n\tfor ;\n\tSun, 26 Feb 2023 15:41:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13FA462662;\n\tSun, 26 Feb 2023 16:41:39 +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 0074362661\n\tfor ;\n\tSun, 26 Feb 2023 16:41:36 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C4216DB;\n\tSun, 26 Feb 2023 16:41:36 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677426099;\n\tbh=QohIxHw4MCCvompDQQta3ZjCqHpOXaF/rbEi1ikhwp4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YNIR4nLdbTgCIkmaJ1cV/AEDPkihegDImbVwpLZgCJKfL0CB5TmHegOdsg1MJYQ0D\n\tZ8rBraV74CdOR2XQ5FZ+7QTg91RyEJ3i5mgHkv7HyozWmO3TPSPyQZ+cFRELNeO+9k\n\tTXbvsWCIsTdCAHu1dbHY2GyuWQ8GaBbT25LRWPMb2+s0F6r3o01AeGVhmUWMLn9+5n\n\tCJyPYTKNNDhFd43T7fpICzLkxOZN2so0qNcTsI5Xn/i9wsGr1g8FEC3sZJKaq63wdO\n\trcVyd8aOyirmSMp91sIGBKJzqGbdCAlHcEVeFse1VI0F6mdmBDidogSUGCNlsm+iMo\n\togaTRtkVK6LYw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677426096;\n\tbh=QohIxHw4MCCvompDQQta3ZjCqHpOXaF/rbEi1ikhwp4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kdlWM5o3OrPkxS535ZjpOe/8bucL/83WWceYh2vj2vhwEkOTmZMeAFdqFh+IW/51y\n\tp5QFPQnxv7rVZ98WvLHHUb21dW4CcI/10w85lpIXdbmn7XXeErhe9KQy933QoCS1ab\n\tEjcTJIgNMdhSuZPzRmZS0ak1WD0jIMRYZZ/1A540="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kdlWM5o3\"; dkim-atps=neutral","Date":"Sun, 26 Feb 2023 17:41:36 +0200","To":"Umang Jain ","Message-ID":"","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>\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] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Suhrid Subramaniam ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26483,"web_url":"https://patchwork.libcamera.org/comment/26483/","msgid":"<167749708045.93391.2093428878912164666@Monstersaurus>","date":"2023-02-27T11:24:40","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36)\n> On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote:\n> > Hi Suhrid,\n> > \n> > Thank you for the patch.\n> > \n> > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> > > - If no converter is found, converter_ becomes a nullptr and\n> > > !converter_->isValid() causes a segmentation fault.\n> > > - Avoid this by checking if converter_ is a nullptr.\n> > >\n> > > Signed-off-by: Suhrid Subramaniam \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 24ded4db..2423ec10 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> > > MediaDevice *converter = pipe->converter();\n> > > if (converter) {\n> > > converter_ = ConverterFactoryBase::create(converter);\n> > > - if (!converter_->isValid()) {\n> > > + if (!converter_) {\n> > \n> > it seems this change will drop the validity check so maybe :\n> >         if (!converter_ && converter_->isValid()) {\n> > \n> > The error message below might be need to get tweaked a bit.\n> > \n> > On the other hand, I wonder if ConverterFactoryBase::create() should be \n> > allowed? to return a non-valid converter pointer or not.\n> \n> I don't see a use case for that, so I'd rather check for !valid in\n> ::create() and return nullptr in that case.\n\nYup - that answers my question. If we don't use isValid() it should be\ndropped.\n\nBut if it's removed here, but should be kept, please add it into\n::create(). Probably as a patch preceeding this one.\n\n--\nKieran\n\n\n> \n> > > LOG(SimplePipeline, Warning)\n> > > << \"Failed to create converter, disabling format conversion\";\n> > > converter_.reset();\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 517D7BE080\n\tfor ;\n\tMon, 27 Feb 2023 11:24:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E17862676;\n\tMon, 27 Feb 2023 12:24:44 +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 CA2506261A\n\tfor ;\n\tMon, 27 Feb 2023 12:24:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A68456A;\n\tMon, 27 Feb 2023 12:24:42 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677497084;\n\tbh=2CNqSM647f0rSa+uekv8/S3Lcbw/ozAkMr1dVcC6jt8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=o5reqbleN/3hu+fRwPxbupDrw0Jocc3wbl81bBZFmPUGISq6zvDSghL8/MfZ8PfAr\n\tKzphLX37uc6b322XewhDGtsGBqz+Rpuf5oIVFsyb57xXvXQYhbBAMeYfAGokZyC65w\n\t2UnTsyK6gmrY6bFK8nCQQVqnEiOnjVuQGbhKmv+sVpIT5yarsB7JlZ14Jqvm/Pkcqc\n\tqDWR0F5VAHVMSfAeUFcJGTE81tXn0tbhhpaPlzQuO3P3Z1aY4ICcb20jXoA8FKuX2l\n\tapdvQp99HdR2Dy72ZHKIePbOJXFT02407F8n0nwMWhFNFR2xHnKfnaa9/g9xszQbql\n\tAB2Go0sQEbVTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677497082;\n\tbh=2CNqSM647f0rSa+uekv8/S3Lcbw/ozAkMr1dVcC6jt8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=j0yLcf339Y8yhrktJ2lZyOHQLGVT355rpOmjaIYxoM0wC/xDePDrsvplrWTsc8l2+\n\tN+f3/i9qBTY1yqlWkqJgCfjb5B3C1BKt/ig+oIN3yFEZDmlbRGyY6jO+0qTmI9V6zd\n\tNnLSrGxYZopN0PjbGgisyjqi6sgkcNyj23w9tx0o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"j0yLcf33\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>\n\t\n\t","To":"Laurent Pinchart ,\n\tLaurent Pinchart via libcamera-devel\n\t, \n\tUmang Jain ","Date":"Mon, 27 Feb 2023 11:24:40 +0000","Message-ID":"<167749708045.93391.2093428878912164666@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Suhrid Subramaniam ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26486,"web_url":"https://patchwork.libcamera.org/comment/26486/","msgid":"","date":"2023-02-27T20:28:51","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Laurent, Kieran,\n\nThank you.\nI will add another patch to check the isValid() condition in ::create() itself.\n\nSuhrid.\n\n\n-----Original Message-----\nFrom: Kieran Bingham \nSent: Monday, February 27, 2023 3:25 AM\nTo: Laurent Pinchart ; Laurent Pinchart via libcamera-devel ; Umang Jain \nCc: Suhrid Subramaniam ; libcamera-devel@lists.libcamera.org\nSubject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr\n\nQuoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36)\n> On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote:\n> > Hi Suhrid,\n> > \n> > Thank you for the patch.\n> > \n> > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> > > - If no converter is found, converter_ becomes a nullptr and\n> > > !converter_->isValid() causes a segmentation fault.\n> > > - Avoid this by checking if converter_ is a nullptr.\n> > >\n> > > Signed-off-by: Suhrid Subramaniam \n> > > \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp \n> > > b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 24ded4db..2423ec10 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> > > MediaDevice *converter = pipe->converter();\n> > > if (converter) {\n> > > converter_ = ConverterFactoryBase::create(converter);\n> > > - if (!converter_->isValid()) {\n> > > + if (!converter_) {\n> > \n> > it seems this change will drop the validity check so maybe :\n> >         if (!converter_ && converter_->isValid()) {\n> > \n> > The error message below might be need to get tweaked a bit.\n> > \n> > On the other hand, I wonder if ConverterFactoryBase::create() should \n> > be allowed? to return a non-valid converter pointer or not.\n> \n> I don't see a use case for that, so I'd rather check for !valid in\n> ::create() and return nullptr in that case.\n\nYup - that answers my question. If we don't use isValid() it should be dropped.\n\nBut if it's removed here, but should be kept, please add it into ::create(). Probably as a patch preceeding this one.\n\n--\nKieran\n\n\n> \n> > > LOG(SimplePipeline, Warning)\n> > > << \"Failed to create converter, disabling format conversion\";\n> > > converter_.reset();\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","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 CECEFBE080\n\tfor ;\n\tMon, 27 Feb 2023 20:29:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CDDE62676;\n\tMon, 27 Feb 2023 21:29:16 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677529756;\n\tbh=UlMedioSWn4JoN5tU2hjoeupqy1YPaaqTb2Bwe5EK/g=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=d/PiZaaqtsPLRqJFRv4axyeHF7Z/gSUwl3uhdPy6+zJ/IhwAaGsVzwczG4nJ+VUwQ\n\tP/j2NVXuvWGSkWmMP1ndMzgFi+JMoJhrEeMd3n4dVtfJjt0DsfZNc563T3x7gv/oMO\n\tUIS+UOshBwpRlrUN0eoXuFALBqfiodODmVj/o/p2gtK5VQlg/8rQhMwR4p8aMehzPr\n\t0AhgUQERoiDZsVCX2GuvtJ431GTZOPBVQKYuTyJeAIiqo3Hojuk6KsjZ3d2qU7+bxF\n\tmr3V5V3hze714EwLYhHwcJbyUEnYaNTH7uZV9Ky7yQ8DsAVpvDq2loD4nPLqyeD047\n\tpCfUlWiD1BOag==","To":"Kieran Bingham , Laurent Pinchart\n\t, Laurent Pinchart via libcamera-devel\n\t, Umang Jain\n\t","Date":"Mon, 27 Feb 2023 20:28:51 +0000","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>\n\t\n\t\n\t<167749708045.93391.2093428878912164666@Monstersaurus>","In-Reply-To":"<167749708045.93391.2093428878912164666@Monstersaurus>","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Suhrid Subramaniam via libcamera-devel\n\t","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Suhrid Subramaniam ","List-Help":"","Subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26487,"web_url":"https://patchwork.libcamera.org/comment/26487/","msgid":"","date":"2023-02-27T20:28:51","subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Laurent, Kieran,\n\nThank you.\nI will add another patch to check the isValid() condition in ::create() itself.\n\nSuhrid.\n\n\n-----Original Message-----\nFrom: Kieran Bingham \nSent: Monday, February 27, 2023 3:25 AM\nTo: Laurent Pinchart ; Laurent Pinchart via libcamera-devel ; Umang Jain \nCc: Suhrid Subramaniam ; libcamera-devel@lists.libcamera.org\nSubject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr\n\nQuoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36)\n> On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote:\n> > Hi Suhrid,\n> > \n> > Thank you for the patch.\n> > \n> > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote:\n> > > - If no converter is found, converter_ becomes a nullptr and\n> > > !converter_->isValid() causes a segmentation fault.\n> > > - Avoid this by checking if converter_ is a nullptr.\n> > >\n> > > Signed-off-by: Suhrid Subramaniam \n> > > \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp \n> > > b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 24ded4db..2423ec10 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> > > MediaDevice *converter = pipe->converter();\n> > > if (converter) {\n> > > converter_ = ConverterFactoryBase::create(converter);\n> > > - if (!converter_->isValid()) {\n> > > + if (!converter_) {\n> > \n> > it seems this change will drop the validity check so maybe :\n> >         if (!converter_ && converter_->isValid()) {\n> > \n> > The error message below might be need to get tweaked a bit.\n> > \n> > On the other hand, I wonder if ConverterFactoryBase::create() should \n> > be allowed? to return a non-valid converter pointer or not.\n> \n> I don't see a use case for that, so I'd rather check for !valid in\n> ::create() and return nullptr in that case.\n\nYup - that answers my question. If we don't use isValid() it should be dropped.\n\nBut if it's removed here, but should be kept, please add it into ::create(). Probably as a patch preceeding this one.\n\n--\nKieran\n\n\n> \n> > > LOG(SimplePipeline, Warning)\n> > > << \"Failed to create converter, disabling format conversion\";\n> > > converter_.reset();\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","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 59DD6BE080\n\tfor ;\n\tMon, 27 Feb 2023 22:22:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9A7C62684;\n\tMon, 27 Feb 2023 23:22:53 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677536573;\n\tbh=+VxaB99YlwhpYe+LP/mXItXbQa7wYRcQ8v0MtDWgm3U=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=Au7GhyA23+QH4LNajGBKntb0up4dNXeGgd1gn68QgqilolFW38OG6x0p12Z06f5YF\n\tFHadxxcYNmkFJrgmozrpSCpu4FPajVmReSS7K8seW6K4IvY7NrhXSXrygVDUsuK3s1\n\tZcy8JsetnJWvOhQcDh3cQMYOU3279ESghYKf3a2Df1F4ODSCcg00bvBzj89XL8R+8c\n\tHPeeinip51lSBOEEqDVPnob3nyqOngCGWWUhFjU6YVUzBFuTkMN/WZAAM1LsianY3n\n\tUlI3RWkGw4JTZuXt1uZpCtWPoyYcd2a7odIa0oSR5+7TuWRFyLJmoKbNdF3cyFzUC0\n\tHsRgl0Zgcr6xA==","To":"Kieran Bingham , Laurent Pinchart\n\t, Laurent Pinchart via libcamera-devel\n\t, Umang Jain\n\t","Date":"Mon, 27 Feb 2023 20:28:51 +0000","References":"<20230223204755.46740-1-suhrid.subramaniam@mediatek.com>\n\t\n\t\n\t<167749708045.93391.2093428878912164666@Monstersaurus>","In-Reply-To":"<167749708045.93391.2093428878912164666@Monstersaurus>","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Suhrid Subramaniam via libcamera-devel\n\t","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Suhrid Subramaniam ","List-Help":"","Subject":"Re: [libcamera-devel] libcamera: pipeline: simple: Check if\n\tconverter_ is a nullptr","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]