[libcamera-devel,13/14] libcamera: pipeline: rkisp1: Factorize errors handling path on start()
diff mbox series

Message ID 20220908184850.1874303-14-xavier.roumegue@oss.nxp.com
State Changes Requested
Headers show
Series
  • Add dw100 dewarper support to simple/rkisp1 pipeline
Related show

Commit Message

Xavier Roumegue Sept. 8, 2022, 6:48 p.m. UTC 
Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 45 +++++++++++-------------
 1 file changed, 21 insertions(+), 24 deletions(-)

Comments

Laurent Pinchart Oct. 4, 2022, 2:18 a.m. UTC | #1 
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@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;
>  }
>
Xavier Roumegue Oct. 7, 2022, 1:17 p.m. UTC | #2 
Hi Laurent,


On 10/4/22 04:18, Laurent Pinchart wrote:
> 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@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 ?

Thanks for this smarter and more elegant proposition. Indeed, this 
solves my problem and your series got my R-B (for what it's worth :) )

I will use it in the v2 patchset.

Xavier
> 
>>   	}
>>   
>>   	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;
>>   }
>>   
>

Patch
diff mbox series 

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;
 	}
 
 	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;
 }