<pre>
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

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->