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

[Paul Elder]

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.


Paul

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