[libcamera-devel] [PATCH 13/14] libcamera: pipeline: rkisp1: Factorize errors handling path on start()

[Laurent Pinchart]

Hi Xavier,

Thank you for the patch.

On Thu, Sep 08, 2022 at 08:48:49PM +0200, Xavier Roumegue via libcamera-devel wrote:
> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 45 +++++++++++-------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 25fbf9f1..c1522ca6 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -828,59 +828,56 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  
>  	ret = data->ipa_->start();
>  	if (ret) {
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start IPA " << camera->id();
> -		return ret;
> +		goto ipa_err;

Goto's are not great, as they can't cross local variable declarations.
I've sent a patch that adds a new utils::Cleaner class ("[PATCH 1/2]
libcamera: utils: Add Cleaner class") with a sample of how it should be
used in this function ("[PATCH 2/2] pipeline: rkisp1: Use utils::Cleaner
to simplify error handling in start"). Could you have a look and tell me
if it solves your problem ?

>  	}
>  
>  	data->frame_ = 0;
>  
>  	ret = param_->streamOn();
>  	if (ret) {
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start parameters " << camera->id();
> -		return ret;
> +		goto param_err;
>  	}
>  
>  	ret = stat_->streamOn();
>  	if (ret) {
> -		param_->streamOff();
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start statistics " << camera->id();
> -		return ret;
> +		goto stat_err;
>  	}
>  
>  	if (data->mainPath_->isEnabled()) {
>  		ret = mainPath_.start();
> -		if (ret) {
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> -			return ret;
> -		}
> +		if (ret)
> +			goto main_err;
>  	}
>  
>  	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
>  		ret = selfPath_.start();
> -		if (ret) {
> -			mainPath_.stop();
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> -			return ret;
> -		}
> +		if (ret)
> +			goto self_err;
>  	}
>  
>  	isp_->setFrameStartEnabled(true);
>  
>  	activeCamera_ = camera;
> +	goto no_err;
> +
> +self_err:
> +	if (data->mainPath_->isEnabled())
> +		mainPath_.stop();
> +main_err:
> +	stat_->streamOff();
> +stat_err:
> +	param_->streamOff();
> +param_err:
> +	data->ipa_->stop();
> +ipa_err:
> +	freeBuffers(camera);
> +no_err:
>  	return ret;
>  }
>  

-- 
Regards,

Laurent Pinchart