[libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr

[Suhrid Subramaniam]

An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230227/8a3553df/attachment-0001.htm>
-------------- next part --------------
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 at ideasonboard.com> 
Sent: Monday, February 27, 2023 3:25 AM
To: Laurent Pinchart <laurent.pinchart at ideasonboard.com>; Laurent Pinchart via libcamera-devel <libcamera-devel at lists.libcamera.org>; Umang Jain <umang.jain at ideasonboard.com>
Cc: Suhrid Subramaniam <Suhrid.Subramaniam at mediatek.com>; libcamera-devel at 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 at 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