| Message ID | 20230302190629.12506-2-suhrid.subramaniam@mediatek.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Suhrid, Thank you for the patch. On Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via libcamera-devel wrote: > - In cases where ConverterFactoryBase::create returns a nullptr, > converter_->isValid() causes a segmentation fault. > > - Solve this by checking if converter_ is a nullptr. > > - Additionally, check for converter validity in the create function itself > and return a nullptr if the converter is invalid. The commit message can be improved by avoiding the bullet list: The ConverterFactoryBase::create() function returns a nullptr when no converter is found. The only caller, SimpleCameraData::init(), checks if the converter is valid with isValid(), but doesn't check if the pointer is null, which can lead to a crash. We could check both pointer validity and converter validity in the caller, but to limit the complexity in callers, it is better to check the converter validity in the create() function and return a null pointer when no valid converter is found. > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > --- > src/libcamera/converter.cpp | 3 ++- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > index 3de39cff..38141da6 100644 > --- a/src/libcamera/converter.cpp > +++ b/src/libcamera/converter.cpp > @@ -226,8 +226,9 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media) > << "Creating converter from " > << factory->name_ << " factory with " > << (it == compatibles.end() ? "no" : media->driver()) << " alias."; > + std::unique_ptr<Converter> converter_ = factory->createInstance(media); The variable should be named 'converter' as it's a local variable. We use trailing underscores for class members only. > > - return factory->createInstance(media); > + return converter_->isValid() ? std::move(converter_) : nullptr; The std::move() can prevent return value optimization. It would be best to write if (!converter_->isValid()) return nullptr; return converter_; We could even keep iterating over the other factories: if (converter_->isValid()) return converter; It may not be very useful at this point, but it wouldn't hurt and could possibly help later. If you're fine with these changes, there's no need to submit a new version, I can apply them locally. > } > > return nullptr; > 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();
Hi Laurent, Yes, these changes do make sense and look good to me : ) Thanks for the explanations too! They're very useful to me.
Hi Laurent, Yes, these changes do make sense and look good to me : ) Thanks for the explanations too! They're very useful to me. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Sunday, March 5, 2023 12:48 PM > To: Suhrid Subramaniam <suhridsubramaniam@gmail.com> > Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam > <Suhrid.Subramaniam@mediatek.com> > Subject: Re: [libcamera-devel] [libcamera-devel, v3, 1/1] libcamera: converter: > Check converter validity > > Hi Suhrid, > > Thank you for the patch. > > On Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via > libcamera-devel wrote: > > - In cases where ConverterFactoryBase::create returns a nullptr, > > converter_->isValid() causes a segmentation fault. > > > > - Solve this by checking if converter_ is a nullptr. > > > > - Additionally, check for converter validity in the create function itself > > and return a nullptr if the converter is invalid. > > The commit message can be improved by avoiding the bullet list: > > The ConverterFactoryBase::create() function returns a nullptr when no > converter is found. The only caller, SimpleCameraData::init(), checks if the > converter is valid with isValid(), but doesn't check if the pointer is null, > which can lead to a crash. > > We could check both pointer validity and converter validity in the caller, but > to limit the complexity in callers, it is better to check the converter validity > in the create() function and return a null pointer when no valid converter is > found. > > > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> > > --- > > src/libcamera/converter.cpp | 3 ++- > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp > > index 3de39cff..38141da6 100644 > > --- a/src/libcamera/converter.cpp > > +++ b/src/libcamera/converter.cpp > > @@ -226,8 +226,9 @@ std::unique_ptr<Converter> > ConverterFactoryBase::create(MediaDevice *media) > > << "Creating converter from " > > << factory->name_ << " factory with " > > << (it == compatibles.end() ? "no" : media->driver()) > << " > > alias."; > > + std::unique_ptr<Converter> converter_ = > > +factory->createInstance(media); > > The variable should be named 'converter' as it's a local variable. We use > trailing underscores for class members only. > > > > > - return factory->createInstance(media); > > + return converter_->isValid() ? std::move(converter_) : > nullptr; > > The std::move() can prevent return value optimization. It would be best to > write > > if (!converter_->isValid()) > return nullptr; > > return converter_; > > We could even keep iterating over the other factories: > > if (converter_->isValid()) > return converter; > > It may not be very useful at this point, but it wouldn't hurt and could > possibly help later. > > If you're fine with these changes, there's no need to submit a new version, I > can apply them locally. > > > } > > > > return nullptr; > > 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(); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp index 3de39cff..38141da6 100644 --- a/src/libcamera/converter.cpp +++ b/src/libcamera/converter.cpp @@ -226,8 +226,9 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media) << "Creating converter from " << factory->name_ << " factory with " << (it == compatibles.end() ? "no" : media->driver()) << " alias."; + std::unique_ptr<Converter> converter_ = factory->createInstance(media); - return factory->createInstance(media); + return converter_->isValid() ? std::move(converter_) : nullptr; } return nullptr; 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();
- In cases where ConverterFactoryBase::create returns a nullptr, converter_->isValid() causes a segmentation fault. - Solve this by checking if converter_ is a nullptr. - Additionally, check for converter validity in the create function itself and return a nullptr if the converter is invalid. Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com> --- src/libcamera/converter.cpp | 3 ++- src/libcamera/pipeline/simple/simple.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)