| Message ID | 20230223204755.46740-1-suhrid.subramaniam@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Suhrid, Thank you for the patch. On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > - If no converter is found, converter_ becomes a nullptr and > !converter_->isValid() causes a segmentation fault. > - Avoid this by checking if converter_ is a nullptr. > > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 24ded4db..2423ec10 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > MediaDevice *converter = pipe->converter(); > if (converter) { > converter_ = ConverterFactoryBase::create(converter); > - if (!converter_->isValid()) { > + if (!converter_) { it seems this change will drop the validity check so maybe : if (!converter_ && converter_->isValid()) { The error message below might be need to get tweaked a bit. On the other hand, I wonder if ConverterFactoryBase::create() should be allowed? to return a non-valid converter pointer or not. > LOG(SimplePipeline, Warning) > << "Failed to create converter, disabling format conversion"; > converter_.reset();
Hi Umang, Thanks for reviewing the patch. This is my first time using git-email so please let me know if I need to change my formatting : ) My comments are inline. -----Original Message----- From: Umang Jain <umang.jain@ideasonboard.com> Sent: Thursday, February 23, 2023 10:23 PM To: Suhrid Subramaniam <suhridsubramaniam@gmail.com>; libcamera-devel@lists.libcamera.org Cc: Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com> Subject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr Hi Suhrid, Thank you for the patch. On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > - If no converter is found, converter_ becomes a nullptr and > !converter_->isValid() causes a segmentation fault. > - Avoid this by checking if converter_ is a nullptr. > > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp > b/src/libcamera/pipeline/simple/simple.cpp > index 24ded4db..2423ec10 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > MediaDevice *converter = pipe->converter(); > if (converter) { > converter_ = ConverterFactoryBase::create(converter); > - if (!converter_->isValid()) { > + if (!converter_) { it seems this change will drop the validity check so maybe : if (!converter_ && converter_->isValid()) { For situations where converter_ is a nullptr, the && operation will execute converter_->isValid() and generate a segmentation fault. I'd originally considered updating the converter_ check to the following: if(!converter_ || converter_->isValid()) Laurent said that it probably doesn't make much sense for the converter factory to return a non-null but invalid converter. The error message below might be need to get tweaked a bit. Yep sure. Any specific format you recommend for the error message? On the other hand, I wonder if ConverterFactoryBase::create() should be allowed? to return a non-valid converter pointer or not. > LOG(SimplePipeline, Warning) > << "Failed to create converter, disabling format conversion"; > converter_.reset(); I 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? Thanks!
On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote: > Hi Suhrid, > > Thank you for the patch. > > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > > - If no converter is found, converter_ becomes a nullptr and > > !converter_->isValid() causes a segmentation fault. > > - Avoid this by checking if converter_ is a nullptr. > > > > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 24ded4db..2423ec10 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > > MediaDevice *converter = pipe->converter(); > > if (converter) { > > converter_ = ConverterFactoryBase::create(converter); > > - if (!converter_->isValid()) { > > + if (!converter_) { > > it seems this change will drop the validity check so maybe : > if (!converter_ && converter_->isValid()) { > > The error message below might be need to get tweaked a bit. > > On the other hand, I wonder if ConverterFactoryBase::create() should be > allowed? to return a non-valid converter pointer or not. I don't see a use case for that, so I'd rather check for !valid in ::create() and return nullptr in that case. > > LOG(SimplePipeline, Warning) > > << "Failed to create converter, disabling format conversion"; > > converter_.reset();
Quoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36) > On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote: > > Hi Suhrid, > > > > Thank you for the patch. > > > > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > > > - If no converter is found, converter_ becomes a nullptr and > > > !converter_->isValid() causes a segmentation fault. > > > - Avoid this by checking if converter_ is a nullptr. > > > > > > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 24ded4db..2423ec10 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > > > MediaDevice *converter = pipe->converter(); > > > if (converter) { > > > converter_ = ConverterFactoryBase::create(converter); > > > - if (!converter_->isValid()) { > > > + if (!converter_) { > > > > it seems this change will drop the validity check so maybe : > > if (!converter_ && converter_->isValid()) { > > > > The error message below might be need to get tweaked a bit. > > > > On the other hand, I wonder if ConverterFactoryBase::create() should be > > allowed? to return a non-valid converter pointer or not. > > I don't see a use case for that, so I'd rather check for !valid in > ::create() and return nullptr in that case. Yup - that answers my question. If we don't use isValid() it should be dropped. But if it's removed here, but should be kept, please add it into ::create(). Probably as a patch preceeding this one. -- Kieran > > > > LOG(SimplePipeline, Warning) > > > << "Failed to create converter, disabling format conversion"; > > > converter_.reset(); > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Kieran, Thank you. I will add another patch to check the isValid() condition in ::create() itself. Suhrid. -----Original Message----- From: Kieran Bingham <kieran.bingham@ideasonboard.com> Sent: Monday, February 27, 2023 3:25 AM To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Laurent Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org>; Umang Jain <umang.jain@ideasonboard.com> Cc: Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>; libcamera-devel@lists.libcamera.org Subject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr Quoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36) > On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote: > > Hi Suhrid, > > > > Thank you for the patch. > > > > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > > > - If no converter is found, converter_ becomes a nullptr and > > > !converter_->isValid() causes a segmentation fault. > > > - Avoid this by checking if converter_ is a nullptr. > > > > > > Signed-off-by: Suhrid Subramaniam > > > <suhrid.subramaniam@mediatek.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp > > > b/src/libcamera/pipeline/simple/simple.cpp > > > index 24ded4db..2423ec10 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > > > MediaDevice *converter = pipe->converter(); > > > if (converter) { > > > converter_ = ConverterFactoryBase::create(converter); > > > - if (!converter_->isValid()) { > > > + if (!converter_) { > > > > it seems this change will drop the validity check so maybe : > > if (!converter_ && converter_->isValid()) { > > > > The error message below might be need to get tweaked a bit. > > > > On the other hand, I wonder if ConverterFactoryBase::create() should > > be allowed? to return a non-valid converter pointer or not. > > I don't see a use case for that, so I'd rather check for !valid in > ::create() and return nullptr in that case. Yup - that answers my question. If we don't use isValid() it should be dropped. But if it's removed here, but should be kept, please add it into ::create(). Probably as a patch preceeding this one. -- Kieran > > > > LOG(SimplePipeline, Warning) > > > << "Failed to create converter, disabling format conversion"; > > > converter_.reset(); > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Kieran, Thank you. I will add another patch to check the isValid() condition in ::create() itself. Suhrid. -----Original Message----- From: Kieran Bingham <kieran.bingham@ideasonboard.com> Sent: Monday, February 27, 2023 3:25 AM To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Laurent Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org>; Umang Jain <umang.jain@ideasonboard.com> Cc: Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com>; libcamera-devel@lists.libcamera.org Subject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr Quoting Laurent Pinchart via libcamera-devel (2023-02-26 15:41:36) > On Fri, Feb 24, 2023 at 11:52:35AM +0530, Umang Jain via libcamera-devel wrote: > > Hi Suhrid, > > > > Thank you for the patch. > > > > On 2/24/23 2:17 AM, Suhrid Subramaniam via libcamera-devel wrote: > > > - If no converter is found, converter_ becomes a nullptr and > > > !converter_->isValid() causes a segmentation fault. > > > - Avoid this by checking if converter_ is a nullptr. > > > > > > Signed-off-by: Suhrid Subramaniam > > > <suhrid.subramaniam@mediatek.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp > > > b/src/libcamera/pipeline/simple/simple.cpp > > > index 24ded4db..2423ec10 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -493,7 +493,7 @@ int SimpleCameraData::init() > > > MediaDevice *converter = pipe->converter(); > > > if (converter) { > > > converter_ = ConverterFactoryBase::create(converter); > > > - if (!converter_->isValid()) { > > > + if (!converter_) { > > > > it seems this change will drop the validity check so maybe : > > if (!converter_ && converter_->isValid()) { > > > > The error message below might be need to get tweaked a bit. > > > > On the other hand, I wonder if ConverterFactoryBase::create() should > > be allowed? to return a non-valid converter pointer or not. > > I don't see a use case for that, so I'd rather check for !valid in > ::create() and return nullptr in that case. Yup - that answers my question. If we don't use isValid() it should be dropped. But if it's removed here, but should be kept, please add it into ::create(). Probably as a patch preceeding this one. -- Kieran > > > > LOG(SimplePipeline, Warning) > > > << "Failed to create converter, disabling format conversion"; > > > converter_.reset(); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 24ded4db..2423ec10 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -493,7 +493,7 @@ int SimpleCameraData::init() MediaDevice *converter = pipe->converter(); if (converter) { converter_ = ConverterFactoryBase::create(converter); - if (!converter_->isValid()) { + if (!converter_) { LOG(SimplePipeline, Warning) << "Failed to create converter, disabling format conversion"; converter_.reset();
- If no converter is found, converter_ becomes a nullptr and !converter_->isValid() causes a segmentation fault. - Avoid this by checking if converter_ is a nullptr. Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> --- src/libcamera/pipeline/simple/simple.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)