[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/20230224/df5ee68c/attachment.htm>
-------------- next part --------------
Hi Umang,

Thanks for reviewing the patch. This is my first time using git-email so please let me know if I need to change my formatting : )
My comments are inline.


-----Original Message-----
From: Umang Jain <umang.jain at ideasonboard.com> 
Sent: Thursday, February 23, 2023 10:23 PM
To: Suhrid Subramaniam <suhridsubramaniam at gmail.com>; libcamera-devel at lists.libcamera.org
Cc: Suhrid Subramaniam <Suhrid.Subramaniam at mediatek.com>
Subject: Re: [libcamera-devel] libcamera: pipeline: simple: Check if converter_ is a nullptr

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()) {

For situations where converter_ is a nullptr, the && operation will execute converter_->isValid() and generate a segmentation fault.
I'd originally considered updating the converter_ check to the following:
         if(!converter_ || converter_->isValid())
Laurent said that it probably doesn't make much sense for the converter factory to return a non-null but invalid converter.


The error message below might be need to get tweaked a bit.

Yep sure. Any specific format you recommend for the error message?

On the other hand, I wonder if ConverterFactoryBase::create() should be allowed? to return a non-valid converter pointer or not.
>   			LOG(SimplePipeline, Warning)
>   				<< "Failed to create converter, disabling format conversion";
>   			converter_.reset();

I was thinking of some sort of exception handling/ modification to isValid but the former needs to be implemented in simple.cpp which would have the same effect as !converter_ and the latter would not work if converter_ is a nullptr. Anything you'd recommend?


Thanks!