[libcamera-devel] [libcamera-devel, v3, 1/1] libcamera: converter: Check converter validity

[Suhrid Subramaniam]

An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230305/415959c5/attachment.htm>
-------------- next part --------------
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 at ideasonboard.com>
> Sent: Sunday, March 5, 2023 12:48 PM
> To: Suhrid Subramaniam <suhridsubramaniam at gmail.com>
> Cc: libcamera-devel at lists.libcamera.org; Suhrid Subramaniam
> <Suhrid.Subramaniam at 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 at 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