[libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter: return unique converter only if Valid

[Suhrid Subramaniam]

An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230228/0d53c253/attachment.htm>
-------------- next part --------------
Hi Paul,

Thanks for reviewing the patch.
Okay, I'll squash both patches.

> -----Original Message-----
> From: Paul Elder <paul.elder at ideasonboard.com>
> Sent: Monday, February 27, 2023 11:06 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] [PATCH v2 2/2] libcamera: pipeline: converter:
> return unique converter only if Valid
> 
> On Mon, Feb 27, 2023 at 02:49:10PM -0800, Suhrid Subramaniam via
> libcamera-devel wrote:
> > - Use isValid() to check if m2m_ exists for the selected converter_.
> > - create an instance of the converter only if it is Valid.
> >
> > Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam at mediatek.com>
> > ---
> >  src/libcamera/converter.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index 3de39cff..8a34d068 100644
> > --- a/src/libcamera/converter.cpp
> > +++ b/src/libcamera/converter.cpp
> > @@ -207,8 +207,9 @@ ConverterFactoryBase::ConverterFactoryBase(const
> std::string name, std::initiali
> >   * \param[in] media Name of the factory
> >   *
> >   * \return A unique pointer to a new instance of the converter
> > subclass
> > - * corresponding to the named factory or one of its alias. Otherwise
> > a null
> > - * pointer if no such factory exists
> > + * corresponding to the named factory or one of its alias if the
> > + converter
> > + * instance is valid (checked using isValid()). Otherwise a null
> > + pointer
> > + * if no such factory exists
> >   */
> >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice
> > *media)  { @@ -227,7 +228,7 @@ std::unique_ptr<Converter>
> > ConverterFactoryBase::create(MediaDevice *media)
> >  			<< factory->name_ << " factory with "
> >  			<< (it == compatibles.end() ? "no" : media->driver())
> << "
> > alias.";
> >
> > -		return factory->createInstance(media);
> > +		return factory->createInstance(media)->isValid() ?
> > +factory->createInstance(media) : nullptr;
> 
> Ah, I see, you want to move the isValid() check here. I think you can just
> squash this patch into the previous one.
> 
> Also you're creating two instances if it is valid. I suppose the first one does
> get deconstructed immediately, but still I don't think it's a good idea.
> 

Understood.
So does it make sense to do the following? Or is there a better method you'd recommend?

		<< (it == compatibles.end() ? "no" : media->driver()) << " alias.";

+ 	converter_ = factory->createInstance(media);
-	return factory->createInstance(media);
+	return converter_ ? converter_->isValid() : nullptr;


> 
> Paul
> 
> >  	}
> >
> >  	return nullptr;
> > --
> > 2.39.0
> >