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

[Laurent Pinchart]

Hi Suhrid,

On Tue, Feb 28, 2023 at 06:14:58PM +0000, Suhrid Subramaniam wrote:
> > -----Original Message-----
> > From: Suhrid Subramaniam
> > Sent: Tuesday, February 28, 2023 10:04 AM
> > To: Paul Elder <paul.elder at ideasonboard.com>; Suhrid Subramaniam
> > <suhridsubramaniam at gmail.com>
> > Cc: libcamera-devel at lists.libcamera.org
> > Subject: RE: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: converter:
> > return unique converter only if Valid
> >
> > 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;
> 
> Oops, I meant:
> +return converter_->isValid() ? converter_ : nullptr;

That looks good. As Paul said, I would squash the two patches together.
Otherwise you're introducing a bug in patch 1/2 that you then fix in
2/2.

> > > >  }
> > > >
> > > >  return nullptr;
> 
> ************* 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!

Could you please drop this from mails sent to public mailing lists ?
Technically, it prevents us from merging your code.

-- 
Regards,

Laurent Pinchart