[{"id":26490,"web_url":"https://patchwork.libcamera.org/comment/26490/","msgid":"<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>","date":"2023-02-28T07:05:44","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via libcamera-devel wrote:\n> - Use isValid() to check if m2m_ exists for the selected converter_.\n> - create an instance of the converter only if it is Valid.\n> \n> Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com>\n> ---\n>  src/libcamera/converter.cpp | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 3de39cff..8a34d068 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -207,8 +207,9 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n>   * \\param[in] media Name of the factory\n>   *\n>   * \\return A unique pointer to a new instance of the converter subclass\n> - * corresponding to the named factory or one of its alias. Otherwise a null\n> - * pointer if no such factory exists\n> + * corresponding to the named factory or one of its alias if the converter \n> + * instance is valid (checked using isValid()). Otherwise a null pointer \n> + * if no such factory exists\n>   */\n>  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)\n>  {\n> @@ -227,7 +228,7 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)\n>  \t\t\t<< factory->name_ << \" factory with \"\n>  \t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n>  \n> -\t\treturn factory->createInstance(media);\n> +\t\treturn factory->createInstance(media)->isValid() ? factory->createInstance(media) : nullptr;\n\nAh, I see, you want to move the isValid() check here. I think you can\njust squash this patch into the previous one.\n\nAlso you're creating two instances if it is valid. I suppose the first\none does get deconstructed immediately, but still I don't think it's a\ngood idea.\n\n\nPaul\n\n>  \t}\n>  \n>  \treturn nullptr;\n> -- \n> 2.39.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DF24CBE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Feb 2023 07:05:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56DE362684;\n\tTue, 28 Feb 2023 08:05:55 +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 357EF62676\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Feb 2023 08:05:53 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DE4556A;\n\tTue, 28 Feb 2023 08:05:51 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677567955;\n\tbh=rsSC2xDT4LM4c5s98YMvdJXCsBc9Wm3/ETu+BO/1K/E=;\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=TsRKHe3wOsHSSXcV0qbRIqj1+VU525fv7OEynRudsGvFd16QrVjOJ3tfw00j6JrEL\n\tK+msGheqXXe83yuFN/ZGWDIyAAdelUJK6X9Zp5epon+ljt/JbLZwU8eKgPoFxpYjZv\n\tOjamjjOf87uoGbLzxjbyfpqCFYdVKOrlFPxFB4F2BgapLn/WD8J41PYQZ0Jo1PXjvi\n\t65OOLV3aGvXTO80TJIs5Kx7VcWiHqvxwmcKS85NTowpmogd1rc6dZIrvugMJesW0Xi\n\toIABA3WVok2fJo9/4YaagzOC+K0T/MEa3fD30u6ILund783iLBBKN+IyaeDQR2wuBh\n\t1zQwYalxOSuow==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677567952;\n\tbh=rsSC2xDT4LM4c5s98YMvdJXCsBc9Wm3/ETu+BO/1K/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lOF6tuPevVFguCB0wzcobjjRnup7gZAV56jckfN9k03zEC+dghpNulFKl8pNtQGFa\n\tCkVJlQCzVtzJUvzTdPAcJ0A6XMdZ9S5zvwDDHCOqX9uzW2TOP/0oIcbpf5IxfdvDCZ\n\t/WFgW/QoZ2AWSt+lods2TvvxZ/t49VNdYTs4PFZE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lOF6tuPe\"; dkim-atps=neutral","Date":"Tue, 28 Feb 2023 16:05:44 +0900","To":"Suhrid Subramaniam <suhridsubramaniam@gmail.com>","Message-ID":"<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"Suhrid Subramaniam <suhrid.subramaniam@mediatek.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26500,"web_url":"https://patchwork.libcamera.org/comment/26500/","msgid":"<mailman.37.1677607430.25031.libcamera-devel@lists.libcamera.org>","date":"2023-02-28T18:03:38","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Paul,\n\nThanks for reviewing the patch.\nOkay, I'll squash both patches.\n\n> -----Original Message-----\n> From: Paul Elder <paul.elder@ideasonboard.com>\n> Sent: Monday, February 27, 2023 11:06 PM\n> To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>\n> Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> <Suhrid.Subramaniam@mediatek.com>\n> Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:\n> return unique converter only if Valid\n> \n> On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via\n> libcamera-devel wrote:\n> > - Use isValid() to check if m2m_ exists for the selected converter_.\n> > - create an instance of the converter only if it is Valid.\n> >\n> > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com>\n> > ---\n> >  src/libcamera/converter.cpp | 7 ++++---\n> >  1 file changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > index 3de39cff..8a34d068 100644\n> > --- a/src/libcamera/converter.cpp\n> > +++ b/src/libcamera/converter.cpp\n> > @@ -207,8 +207,9 @@ ConverterFactoryBase::ConverterFactoryBase(const\n> std::string name, std::initiali\n> >   * \\param[in] media Name of the factory\n> >   *\n> >   * \\return A unique pointer to a new instance of the converter\n> > subclass\n> > - * corresponding to the named factory or one of its alias. Otherwise\n> > a null\n> > - * pointer if no such factory exists\n> > + * corresponding to the named factory or one of its alias if the\n> > + converter\n> > + * instance is valid (checked using isValid()). Otherwise a null\n> > + pointer\n> > + * if no such factory exists\n> >   */\n> >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice\n> > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>\n> > ConverterFactoryBase::create(MediaDevice *media)\n> >  \t\t\t<< factory->name_ << \" factory with \"\n> >  \t\t\t<< (it == compatibles.end() ? \"no\" : media->driver())\n> << \"\n> > alias.\";\n> >\n> > -\t\treturn factory->createInstance(media);\n> > +\t\treturn factory->createInstance(media)->isValid() ?\n> > +factory->createInstance(media) : nullptr;\n> \n> Ah, I see, you want to move the isValid() check here. I think you can just\n> squash this patch into the previous one.\n> \n> Also you're creating two instances if it is valid. I suppose the first one does\n> get deconstructed immediately, but still I don't think it's a good idea.\n> \n\nUnderstood.\nSo does it make sense to do the following? Or is there a better method you'd recommend?\n\n\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n\n+ \tconverter_ = factory->createInstance(media);\n-\treturn factory->createInstance(media);\n+\treturn converter_ ? converter_->isValid() : nullptr;\n\n\n> \n> Paul\n> \n> >  \t}\n> >\n> >  \treturn nullptr;\n> > --\n> > 2.39.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 ED09BBE08A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Feb 2023 18:03:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 645F7626A7;\n\tTue, 28 Feb 2023 19:03:50 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677607430;\n\tbh=pYFHe8gfXSpI1o+hbRHg7p1Od2TMIXNza+ttDqBfX+c=;\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=ZmTcnpw1y5gI3l4heZMDef5nAXQqY0K1OH6rP/sJbKQC0QoOzLCTfjLBd6pDZi7uY\n\t4YXorsZa7E3sJy+DCuCEUBVYnmBlxFXkA6HLgxS4M+nqj8Rju/e2r4+B9sWY1hdfaS\n\tJiDHIpJ2aRediq3eFmh9smscG7tBqT/q9EVwKV3x1Tiyshbfyu4x8tMLUnumLk8zfo\n\tVgz2csk1C5mLSUTu79w0IeWOIZRxcv0c/UzbNl7zbbYJizumoVt1H/y0G3rmWUO56c\n\tx0sg6Io8J6fvqjAlq7d9bfFrBCA8Ee28OULKUbhMrogbOaG6OZIC5Aa5fYK3Zs2fCt\n\tgS1pgzgj4c8mQ==","To":"Paul Elder <paul.elder@ideasonboard.com>, Suhrid Subramaniam\n\t<suhridsubramaniam@gmail.com>","Date":"Tue, 28 Feb 2023 18:03:38 +0000","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>\n\t<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>","In-Reply-To":"<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>","MIME-Version":"1.0","Message-ID":"<mailman.37.1677607430.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Suhrid Subramaniam via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t<libcamera-devel@lists.libcamera.org>","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","Reply-To":"Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26501,"web_url":"https://patchwork.libcamera.org/comment/26501/","msgid":"<mailman.38.1677608114.25031.libcamera-devel@lists.libcamera.org>","date":"2023-02-28T18:14:58","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"> -----Original Message-----\n> From: Suhrid Subramaniam\n> Sent: Tuesday, February 28, 2023 10:04 AM\n> To: Paul Elder <paul.elder@ideasonboard.com>; Suhrid Subramaniam\n> <suhridsubramaniam@gmail.com>\n> Cc: libcamera-devel@lists.libcamera.org\n> Subject: RE: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:\n> return unique converter only if Valid\n> \n> Hi Paul,\n> \n> Thanks for reviewing the patch.\n> Okay, I'll squash both patches.\n> \n> > -----Original Message-----\n> > From: Paul Elder <paul.elder@ideasonboard.com>\n> > Sent: Monday, February 27, 2023 11:06 PM\n> > To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>\n> > Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> > <Suhrid.Subramaniam@mediatek.com>\n> > Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n> converter:\n> > return unique converter only if Valid\n> >\n> > On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via\n> > libcamera-devel wrote:\n> > > - Use isValid() to check if m2m_ exists for the selected converter_.\n> > > - create an instance of the converter only if it is Valid.\n> > >\n> > > Signed-off-by: Suhrid Subramaniam\n> <suhrid.subramaniam@mediatek.com>\n> > > ---\n> > >  src/libcamera/converter.cpp | 7 ++++---\n> > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/converter.cpp\n> > > b/src/libcamera/converter.cpp index 3de39cff..8a34d068 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -207,8 +207,9 @@\n> ConverterFactoryBase::ConverterFactoryBase(const\n> > std::string name, std::initiali\n> > >   * \\param[in] media Name of the factory\n> > >   *\n> > >   * \\return A unique pointer to a new instance of the converter\n> > > subclass\n> > > - * corresponding to the named factory or one of its alias.\n> > > Otherwise a null\n> > > - * pointer if no such factory exists\n> > > + * corresponding to the named factory or one of its alias if the\n> > > + converter\n> > > + * instance is valid (checked using isValid()). Otherwise a null\n> > > + pointer\n> > > + * if no such factory exists\n> > >   */\n> > >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice\n> > > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>\n> > > ConverterFactoryBase::create(MediaDevice *media)\n> > >  \t\t\t<< factory->name_ << \" factory with \"\n> > >  \t\t\t<< (it == compatibles.end() ? \"no\" : media->driver())\n> > << \"\n> > > alias.\";\n> > >\n> > > -\t\treturn factory->createInstance(media);\n> > > +\t\treturn factory->createInstance(media)->isValid() ?\n> > > +factory->createInstance(media) : nullptr;\n> >\n> > Ah, I see, you want to move the isValid() check here. I think you can\n> > just squash this patch into the previous one.\n> >\n> > Also you're creating two instances if it is valid. I suppose the first\n> > one does get deconstructed immediately, but still I don't think it's a good\n> idea.\n> >\n> \n> Understood.\n> So does it make sense to do the following? Or is there a better method\n> you'd recommend?\n> \n> \t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \"\n> alias.\";\n> \n> + \tconverter_ = factory->createInstance(media);\n> -\treturn factory->createInstance(media);\n> +\treturn converter_ ? converter_->isValid() : nullptr;\n\nOops, I meant:\n+\treturn converter_->isValid() ? converter_ : nullptr;\n\n\n> \n> \n> >\n> > Paul\n> >\n> > >  \t}\n> > >\n> > >  \treturn nullptr;\n> > > --\n> > > 2.39.0\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3751BBE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Feb 2023 18:15:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A9EE626A9;\n\tTue, 28 Feb 2023 19:15:14 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677608114;\n\tbh=COdHMBf9n2ovf7kpo3cRXlEVusd1ZxhY1hDRTMjC2ng=;\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=tmxBbPchchOB4BWqb8+NV9uYHbPzfnNjoU8tWvPTV5jRvhEJgPuu4PeSxJmjGLtud\n\tgnUpLym0VU6T+wOozMJD+Xk0dPvvE7lHOkXPK0FUJTyCuOj8DWZf/ULEHRKyX39wIM\n\tKRZyUozQxZJ+siywAYg+qvx9ZOh4f3JoZ5C9Fo5yvRNiPclWElOdoUrn50O60I432M\n\tak5ECXGIw1G3/jx1FgW1emo64eHuzb1iS7nN9C6Z9W08fyuKKix3Hcq/Bz8lGmTl3t\n\tTnV3Ov3Rskdh/QLeaNB9TVNhkDATlLmEKwpa0ioqH1eA7iVNpmXxB8m/3/JzJvypsY\n\tu3CEMMEN4SQjw==","To":"Paul Elder <paul.elder@ideasonboard.com>, Suhrid Subramaniam\n\t<suhridsubramaniam@gmail.com>","Date":"Tue, 28 Feb 2023 18:14:58 +0000","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>\n\t<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>\n\t<TY2PR03MB3758BD23064A05B3FF4E4A87E4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>","In-Reply-To":"<TY2PR03MB3758BD23064A05B3FF4E4A87E4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>","MIME-Version":"1.0","Message-ID":"<mailman.38.1677608114.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Suhrid Subramaniam via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t<libcamera-devel@lists.libcamera.org>","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","Reply-To":"Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26502,"web_url":"https://patchwork.libcamera.org/comment/26502/","msgid":"<Y/6KiDGCuP6mRbVf@pendragon.ideasonboard.com>","date":"2023-02-28T23:13:12","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Suhrid,\n\nOn Tue, Feb 28, 2023 at 06:14:58PM +0000, Suhrid Subramaniam wrote:\n> > -----Original Message-----\n> > From: Suhrid Subramaniam\n> > Sent: Tuesday, February 28, 2023 10:04 AM\n> > To: Paul Elder <paul.elder@ideasonboard.com>; Suhrid Subramaniam\n> > <suhridsubramaniam@gmail.com>\n> > Cc: libcamera-devel@lists.libcamera.org\n> > Subject: RE: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:\n> > return unique converter only if Valid\n> >\n> > Hi Paul,\n> >\n> > Thanks for reviewing the patch.\n> > Okay, I'll squash both patches.\n> >\n> > > -----Original Message-----\n> > > From: Paul Elder <paul.elder@ideasonboard.com>\n> > > Sent: Monday, February 27, 2023 11:06 PM\n> > > To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>\n> > > Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> > > <Suhrid.Subramaniam@mediatek.com>\n> > > Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n> > converter:\n> > > return unique converter only if Valid\n> > >\n> > > On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via\n> > > libcamera-devel wrote:\n> > > > - Use isValid() to check if m2m_ exists for the selected converter_.\n> > > > - create an instance of the converter only if it is Valid.\n> > > >\n> > > > Signed-off-by: Suhrid Subramaniam\n> > <suhrid.subramaniam@mediatek.com>\n> > > > ---\n> > > >  src/libcamera/converter.cpp | 7 ++++---\n> > > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/converter.cpp\n> > > > b/src/libcamera/converter.cpp index 3de39cff..8a34d068 100644\n> > > > --- a/src/libcamera/converter.cpp\n> > > > +++ b/src/libcamera/converter.cpp\n> > > > @@ -207,8 +207,9 @@\n> > ConverterFactoryBase::ConverterFactoryBase(const\n> > > std::string name, std::initiali\n> > > >   * \\param[in] media Name of the factory\n> > > >   *\n> > > >   * \\return A unique pointer to a new instance of the converter\n> > > > subclass\n> > > > - * corresponding to the named factory or one of its alias.\n> > > > Otherwise a null\n> > > > - * pointer if no such factory exists\n> > > > + * corresponding to the named factory or one of its alias if the\n> > > > + converter\n> > > > + * instance is valid (checked using isValid()). Otherwise a null\n> > > > + pointer\n> > > > + * if no such factory exists\n> > > >   */\n> > > >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice\n> > > > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>\n> > > > ConverterFactoryBase::create(MediaDevice *media)\n> > > >  << factory->name_ << \" factory with \"\n> > > >  << (it == compatibles.end() ? \"no\" : media->driver())\n> > > << \"\n> > > > alias.\";\n> > > >\n> > > > -return factory->createInstance(media);\n> > > > +return factory->createInstance(media)->isValid() ?\n> > > > +factory->createInstance(media) : nullptr;\n> > >\n> > > Ah, I see, you want to move the isValid() check here. I think you can\n> > > just squash this patch into the previous one.\n> > >\n> > > Also you're creating two instances if it is valid. I suppose the first\n> > > one does get deconstructed immediately, but still I don't think it's a good\n> > idea.\n> > >\n> >\n> > Understood.\n> > So does it make sense to do the following? Or is there a better method\n> > you'd recommend?\n> >\n> > << (it == compatibles.end() ? \"no\" : media->driver()) << \"\n> > alias.\";\n> >\n> > + converter_ = factory->createInstance(media);\n> > -return factory->createInstance(media);\n> > +return converter_ ? converter_->isValid() : nullptr;\n> \n> Oops, I meant:\n> +return converter_->isValid() ? converter_ : nullptr;\n\nThat looks good. As Paul said, I would squash the two patches together.\nOtherwise you're introducing a bug in patch 1/2 that you then fix in\n2/2.\n\n> > > >  }\n> > > >\n> > > >  return nullptr;\n> \n> ************* MEDIATEK Confidentiality Notice ********************\n> The information contained in this e-mail message (including any\n> attachments) may be confidential, proprietary, privileged, or otherwise\n> exempt from disclosure under applicable laws. It is intended to be\n> conveyed only to the designated recipient(s). Any use, dissemination,\n> distribution, printing, retaining or copying of this e-mail (including its\n> attachments) by unintended recipient(s) is strictly prohibited and may\n> be unlawful. If you are not an intended recipient of this e-mail, or believe\n> that you have received this e-mail in error, please notify the sender\n> immediately (by replying to this e-mail), delete any and all copies of\n> this e-mail (including any attachments) from your system, and do not\n> disclose the content of this e-mail to any other person. Thank you!\n\nCould you please drop this from mails sent to public mailing lists ?\nTechnically, it prevents us from merging your code.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 78BCDBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Feb 2023 23:13:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA9256267E;\n\tWed,  1 Mar 2023 00:13:13 +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 8CFAA6267E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Mar 2023 00:13:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(153.162-64-87.adsl-dyn.isp.belgacom.be [87.64.162.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DACBE890;\n\tWed,  1 Mar 2023 00:13:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677625993;\n\tbh=d4ReGvidx3ZBo9/8G3OyyyMDyaZjmBkJMCzbv8yz2+E=;\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=uTGmiZ00tGFAnYs7Yiw68RtMzyfe884FvKb3HmushCTqqEAKF7DcVc/m5FyYu/sNP\n\tHphEWJ5odgcmEleLPHBkcIeuYd/Ngz+0mljckKmGdzduf7EtuNRAHTQKbNxqWf1Xm2\n\tdRmpHxtoq85AmuUMigy2AE4/zrQPskyBRMSMEZc5PlMdAAw4Y+StCpHiPWDsGN6D/g\n\tJ4OVb+77jUDDYuAQzKQjSS1HjIxJip1VPF/IvQ/Tc8UuiKnDcU7LvlpP5k3GVyZ9LP\n\tLHyDKKCNohP2Us5y+n4gpsNJVRVMShOyPWnoEsvVslhtht++0/SUHO2dPzv1j856dv\n\t4A8FC0mUxNOOg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677625991;\n\tbh=d4ReGvidx3ZBo9/8G3OyyyMDyaZjmBkJMCzbv8yz2+E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DLmvOXjCkA+1p/n3PiDB/FK5V01TnOFQwtmcdMobjMW04VbKzfOGQuhY1mDP/R3ZI\n\tAJ9Diy3z5lVRfV0sO5/SiW3znPaRg0jpKF/q7+STP8YLoL7HFsqbgNy2TuviJ7wz3h\n\tHM4YwuzTBNDs+vsTDvPByt24jtC9E9yw9zkY/EKc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DLmvOXjC\"; dkim-atps=neutral","Date":"Wed, 1 Mar 2023 01:13:12 +0200","To":"Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>","Message-ID":"<Y/6KiDGCuP6mRbVf@pendragon.ideasonboard.com>","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>\n\t<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>\n\t<TY2PR03MB3758BD23064A05B3FF4E4A87E4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>\n\t<TY2PR03MB375876158E3CD723B4BD64FCE4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<TY2PR03MB375876158E3CD723B4BD64FCE4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t<libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26504,"web_url":"https://patchwork.libcamera.org/comment/26504/","msgid":"<167762754207.93391.6056607640648714666@Monstersaurus>","date":"2023-02-28T23:39:02","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","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-28 23:13:12)\n> Hi Suhrid,\n> \n> On Tue, Feb 28, 2023 at 06:14:58PM +0000, Suhrid Subramaniam wrote:\n> > > -----Original Message-----\n> > > From: Suhrid Subramaniam\n> > > Sent: Tuesday, February 28, 2023 10:04 AM\n> > > To: Paul Elder <paul.elder@ideasonboard.com>; Suhrid Subramaniam\n> > > <suhridsubramaniam@gmail.com>\n> > > Cc: libcamera-devel@lists.libcamera.org\n> > > Subject: RE: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:\n> > > return unique converter only if Valid\n> > >\n> > > Hi Paul,\n> > >\n> > > Thanks for reviewing the patch.\n> > > Okay, I'll squash both patches.\n> > >\n> > > > -----Original Message-----\n> > > > From: Paul Elder <paul.elder@ideasonboard.com>\n> > > > Sent: Monday, February 27, 2023 11:06 PM\n> > > > To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>\n> > > > Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> > > > <Suhrid.Subramaniam@mediatek.com>\n> > > > Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n> > > converter:\n> > > > return unique converter only if Valid\n> > > >\n> > > > On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via\n> > > > libcamera-devel wrote:\n> > > > > - Use isValid() to check if m2m_ exists for the selected converter_.\n> > > > > - create an instance of the converter only if it is Valid.\n> > > > >\n> > > > > Signed-off-by: Suhrid Subramaniam\n> > > <suhrid.subramaniam@mediatek.com>\n> > > > > ---\n> > > > >  src/libcamera/converter.cpp | 7 ++++---\n> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/converter.cpp\n> > > > > b/src/libcamera/converter.cpp index 3de39cff..8a34d068 100644\n> > > > > --- a/src/libcamera/converter.cpp\n> > > > > +++ b/src/libcamera/converter.cpp\n> > > > > @@ -207,8 +207,9 @@\n> > > ConverterFactoryBase::ConverterFactoryBase(const\n> > > > std::string name, std::initiali\n> > > > >   * \\param[in] media Name of the factory\n> > > > >   *\n> > > > >   * \\return A unique pointer to a new instance of the converter\n> > > > > subclass\n> > > > > - * corresponding to the named factory or one of its alias.\n> > > > > Otherwise a null\n> > > > > - * pointer if no such factory exists\n> > > > > + * corresponding to the named factory or one of its alias if the\n> > > > > + converter\n> > > > > + * instance is valid (checked using isValid()). Otherwise a null\n> > > > > + pointer\n> > > > > + * if no such factory exists\n> > > > >   */\n> > > > >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice\n> > > > > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>\n> > > > > ConverterFactoryBase::create(MediaDevice *media)\n> > > > >  << factory->name_ << \" factory with \"\n> > > > >  << (it == compatibles.end() ? \"no\" : media->driver())\n> > > > << \"\n> > > > > alias.\";\n> > > > >\n> > > > > -return factory->createInstance(media);\n> > > > > +return factory->createInstance(media)->isValid() ?\n> > > > > +factory->createInstance(media) : nullptr;\n> > > >\n> > > > Ah, I see, you want to move the isValid() check here. I think you can\n> > > > just squash this patch into the previous one.\n> > > >\n> > > > Also you're creating two instances if it is valid. I suppose the first\n> > > > one does get deconstructed immediately, but still I don't think it's a good\n> > > idea.\n> > > >\n> > >\n> > > Understood.\n> > > So does it make sense to do the following? Or is there a better method\n> > > you'd recommend?\n> > >\n> > > << (it == compatibles.end() ? \"no\" : media->driver()) << \"\n> > > alias.\";\n> > >\n> > > + converter_ = factory->createInstance(media);\n> > > -return factory->createInstance(media);\n> > > +return converter_ ? converter_->isValid() : nullptr;\n> > \n> > Oops, I meant:\n> > +return converter_->isValid() ? converter_ : nullptr;\n> \n> That looks good. As Paul said, I would squash the two patches together.\n> Otherwise you're introducing a bug in patch 1/2 that you then fix in\n> 2/2.\n\nIndeed, I believe I originally suggested this patch could preceed the\nother so that wouldn't happen - but I'm fine with them being squashed\ntoo.\n\n--\nKieran\n\n\n> \n> > > > >  }\n> > > > >\n> > > > >  return nullptr;\n> > \n> > ************* MEDIATEK Confidentiality Notice ********************\n> > The information contained in this e-mail message (including any\n> > attachments) may be confidential, proprietary, privileged, or otherwise\n> > exempt from disclosure under applicable laws. It is intended to be\n> > conveyed only to the designated recipient(s). Any use, dissemination,\n> > distribution, printing, retaining or copying of this e-mail (including its\n> > attachments) by unintended recipient(s) is strictly prohibited and may\n> > be unlawful. If you are not an intended recipient of this e-mail, or believe\n> > that you have received this e-mail in error, please notify the sender\n> > immediately (by replying to this e-mail), delete any and all copies of\n> > this e-mail (including any attachments) from your system, and do not\n> > disclose the content of this e-mail to any other person. Thank you!\n> \n> Could you please drop this from mails sent to public mailing lists ?\n> Technically, it prevents us from merging your code.\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2C3D4BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Feb 2023 23:39:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90D7A626AE;\n\tWed,  1 Mar 2023 00:39:06 +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 EE2F962691\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Mar 2023 00:39:04 +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 75DC6890;\n\tWed,  1 Mar 2023 00:39:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677627546;\n\tbh=UwKyFY3NB080+z+g0z8256zzcVONoKBfi/zjCrwx1r4=;\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=vwBjvIHGyIkTZh6h89rWcvR+tqJXDFEPOHJQHLXSGmu8qMGLMmJUJ865Atsi9+DTE\n\t9oCXs3p+C3CA5JMmoCYJX+DVlYLEIZO6MpEVPyX4iiP13+9x6yrGIYnfpMFIFnHXej\n\t72q/8eSM7oA3aAG/7o7DTSyJaDnxL7BTDis0bMweZyf5bQa/FbrRBhQZs1rhVDTE4e\n\tv+cRZpESl3eu1Q1xt75Iox9q4xfgT3F0NRXG6IQDTv7XxOwn+aaVImcmNWY+gVf0OK\n\tR4qHFByTMFTo2Gtw18q9s+mw6v+WJt3b2//GXuYI2LbvsmAIP3RfTxuF4SkkPY2zDa\n\taecEk9z0GuVHg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677627544;\n\tbh=UwKyFY3NB080+z+g0z8256zzcVONoKBfi/zjCrwx1r4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=X6o73xouLRNtlgnQdvY1R+xNHvxGFxss38UJjmcqw+LAT7LyZhdOxcbkDhd0/2Vz2\n\tde/MpLH1MciVHVhTbZwu/4QdhRVgBdLevhq14fjKJmmtbgwc1RYB00i7aTMuT5dCOC\n\th3HLiajtr3oJupEMNKitpT7whwwB4lRXI7omm8Dc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X6o73xou\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<Y/6KiDGCuP6mRbVf@pendragon.ideasonboard.com>","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>\n\t<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>\n\t<TY2PR03MB3758BD23064A05B3FF4E4A87E4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>\n\t<TY2PR03MB375876158E3CD723B4BD64FCE4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>\n\t<Y/6KiDGCuP6mRbVf@pendragon.ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tLaurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>, \n\tSuhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>","Date":"Tue, 28 Feb 2023 23:39:02 +0000","Message-ID":"<167762754207.93391.6056607640648714666@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26507,"web_url":"https://patchwork.libcamera.org/comment/26507/","msgid":"<mailman.39.1677631748.25031.libcamera-devel@lists.libcamera.org>","date":"2023-03-01T00:48:52","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"> -----Original Message-----\n> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Sent: Tuesday, February 28, 2023 3:39 PM\n> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Laurent\n> Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org>; Suhrid\n> Subramaniam <Suhrid.Subramaniam@mediatek.com>\n> Cc: libcamera-devel@lists.libcamera.org\n> Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:\n> return unique converter only if Valid\n> \n> Quoting Laurent Pinchart via libcamera-devel (2023-02-28 23:13:12)\n> > Hi Suhrid,\n> >\n> > On Tue, Feb 28, 2023 at 06:14:58PM +0000, Suhrid Subramaniam wrote:\n> > > > -----Original Message-----\n> > > > From: Suhrid Subramaniam\n> > > > Sent: Tuesday, February 28, 2023 10:04 AM\n> > > > To: Paul Elder <paul.elder@ideasonboard.com>; Suhrid Subramaniam\n> > > > <suhridsubramaniam@gmail.com>\n> > > > Cc: libcamera-devel@lists.libcamera.org\n> > > > Subject: RE: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n> converter:\n> > > > return unique converter only if Valid\n> > > >\n> > > > Hi Paul,\n> > > >\n> > > > Thanks for reviewing the patch.\n> > > > Okay, I'll squash both patches.\n> > > >\n> > > > > -----Original Message-----\n> > > > > From: Paul Elder <paul.elder@ideasonboard.com>\n> > > > > Sent: Monday, February 27, 2023 11:06 PM\n> > > > > To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>\n> > > > > Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> > > > > <Suhrid.Subramaniam@mediatek.com>\n> > > > > Subject: Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n> > > > converter:\n> > > > > return unique converter only if Valid\n> > > > >\n> > > > > On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via\n> > > > > libcamera-devel wrote:\n> > > > > > - Use isValid() to check if m2m_ exists for the selected converter_.\n> > > > > > - create an instance of the converter only if it is Valid.\n> > > > > >\n> > > > > > Signed-off-by: Suhrid Subramaniam\n> > > > <suhrid.subramaniam@mediatek.com>\n> > > > > > ---\n> > > > > >  src/libcamera/converter.cpp | 7 ++++---\n> > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/converter.cpp\n> > > > > > b/src/libcamera/converter.cpp index 3de39cff..8a34d068 100644\n> > > > > > --- a/src/libcamera/converter.cpp\n> > > > > > +++ b/src/libcamera/converter.cpp\n> > > > > > @@ -207,8 +207,9 @@\n> > > > ConverterFactoryBase::ConverterFactoryBase(const\n> > > > > std::string name, std::initiali\n> > > > > >   * \\param[in] media Name of the factory\n> > > > > >   *\n> > > > > >   * \\return A unique pointer to a new instance of the\n> > > > > > converter subclass\n> > > > > > - * corresponding to the named factory or one of its alias.\n> > > > > > Otherwise a null\n> > > > > > - * pointer if no such factory exists\n> > > > > > + * corresponding to the named factory or one of its alias if\n> > > > > > + the converter\n> > > > > > + * instance is valid (checked using isValid()). Otherwise a\n> > > > > > + null pointer\n> > > > > > + * if no such factory exists\n> > > > > >   */\n> > > > > >  std::unique_ptr<Converter>\n> > > > > > ConverterFactoryBase::create(MediaDevice\n> > > > > > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>\n> > > > > > ConverterFactoryBase::create(MediaDevice *media)  <<\n> > > > > > factory->name_ << \" factory with \"\n> > > > > >  << (it == compatibles.end() ? \"no\" : media->driver())\n> > > > > << \"\n> > > > > > alias.\";\n> > > > > >\n> > > > > > -return factory->createInstance(media);\n> > > > > > +return factory->createInstance(media)->isValid() ?\n> > > > > > +factory->createInstance(media) : nullptr;\n> > > > >\n> > > > > Ah, I see, you want to move the isValid() check here. I think\n> > > > > you can just squash this patch into the previous one.\n> > > > >\n> > > > > Also you're creating two instances if it is valid. I suppose the\n> > > > > first one does get deconstructed immediately, but still I don't\n> > > > > think it's a good\n> > > > idea.\n> > > > >\n> > > >\n> > > > Understood.\n> > > > So does it make sense to do the following? Or is there a better\n> > > > method you'd recommend?\n> > > >\n> > > > << (it == compatibles.end() ? \"no\" : media->driver()) << \"\n> > > > alias.\";\n> > > >\n> > > > + converter_ = factory->createInstance(media);\n> > > > -return factory->createInstance(media);\n> > > > +return converter_ ? converter_->isValid() : nullptr;\n> > >\n> > > Oops, I meant:\n> > > +return converter_->isValid() ? converter_ : nullptr;\n> >\n> > That looks good. As Paul said, I would squash the two patches together.\n> > Otherwise you're introducing a bug in patch 1/2 that you then fix in\n> > 2/2.\n\n\nWhile trying to compile and check if this works, I came across an interesting compile error regarding unique pointer ownership.\nI'd to modify the code to use std::move() as follows:\n\n+ converter_ = factory->createInstance(media);\n+ return converter_->isValid() ? std::move(converter_) : nullptr;\n\nI hope this is okay.\n\nSecondly,\nInstead of nullptr, shouldn't we return an empty object of Converter unique pointer?\n\n- return nullptr\n+ return std::unique_ptr<Converter>()\n\n\n> \n> Indeed, I believe I originally suggested this patch could preceed the other so\n> that wouldn't happen - but I'm fine with them being squashed too.\n> \n> --\n> Kieran\n> \n> \nHi Kieran,\nGot it. I'll squash the patches : )\n\n> >\n> > > > > >  }\n> > > > > >\n> > > > > >  return nullptr;\n> >\n> > Could you please drop this from mails sent to public mailing lists ?\n> > Technically, it prevents us from merging your code.\n\nYep, done. Sorry about that!\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D1F27BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Mar 2023 00:49:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26E92626B4;\n\tWed,  1 Mar 2023 01:49:09 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677631749;\n\tbh=NF5JBOF8Hrvv8jZAf6G5D0wKflygeD5cVNYn/3nUmeQ=;\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=MvGCn9br+fm9QS62BhrzNrc4QsFbM1DL/6n5QQSdeilC3UBZ2zSpVc6uZ761Qdnbu\n\tc4I+MK/OpJOyOqSJbvV9P3h5/mOlohuvrt7RQl2LyCpfEuPXyDXE8SakVgjkKfOweN\n\tCLJK8QQ7pUWtV5RF9Bci2RFDgtgl+lvf6f9EVcCiNCohvdN8IF4pnSdHpDralf9vX5\n\tXXBC6nN9F2N1HxFC99nnbSA5YdRgxPzft+fqcIH8KLN9vG0O+Jl0CX//2nKD0cVT/d\n\tGT8+NLQFOJ4GhNKprSWFeqBhfdRETJdi12TsZiKbgZkgPrmQfTPgQMUj0kU0kuJ/U4\n\tiSTbf8gqNXmLQ==","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>, Laurent Pinchart\n\t<laurent.pinchart@ideasonboard.com>, Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Date":"Wed, 1 Mar 2023 00:48:52 +0000","References":"<20230227224910.151337-1-suhrid.subramaniam@mediatek.com>\n\t<20230227224910.151337-3-suhrid.subramaniam@mediatek.com>\n\t<Y/2nyDC4qu8MDcZn@pyrite.rasen.tech>\n\t<TY2PR03MB3758BD23064A05B3FF4E4A87E4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>\n\t<TY2PR03MB375876158E3CD723B4BD64FCE4AC9@TY2PR03MB3758.apcprd03.prod.outlook.com>\n\t<Y/6KiDGCuP6mRbVf@pendragon.ideasonboard.com>\n\t<167762754207.93391.6056607640648714666@Monstersaurus>","In-Reply-To":"<167762754207.93391.6056607640648714666@Monstersaurus>","MIME-Version":"1.0","Message-ID":"<mailman.39.1677631748.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Suhrid Subramaniam via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t<libcamera-devel@lists.libcamera.org>","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","Reply-To":"Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline:\n\tconverter: return unique converter only if Valid","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]