[libcamera-devel,2/2] pipeline: rkisp1: Use utils::Cleaner to simplify error handling in start
diff mbox series

Message ID 20221004021842.6345-3-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Simplify error handling with Cleaner class
Related show

Commit Message

Laurent Pinchart Oct. 4, 2022, 2:18 a.m. UTC 
Error handling in the PipelineHandlerRkISP1::start() function is
cumbersome. Simplify it using the utils::Cleaner class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 +++++++++---------------
 1 file changed, 11 insertions(+), 20 deletions(-)

Comments

Xavier Roumegue Oct. 4, 2022, 11:41 a.m. UTC | #1 
Hi Laurent,

Thanks for your patch.

On 10/4/22 04:18, Laurent Pinchart wrote:
> Error handling in the PipelineHandlerRkISP1::start() function is
> cumbersome. Simplify it using the utils::Cleaner class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 +++++++++---------------
>   1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 455ee2a0a711..33f3b0c5ab00 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -823,69 +823,60 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>   int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>   {
>   	RkISP1CameraData *data = cameraData(camera);
> +	utils::Cleaner cleaner;
>   	int ret;
>   
>   	/* Allocate buffers for internal pipeline usage. */
>   	ret = allocateBuffers(camera);
>   	if (ret)
>   		return ret;
> +	cleaner.defer([&]() { freeBuffers(camera); });
>   
>   	ret = data->ipa_->start();
>   	if (ret) {
> -		freeBuffers(camera);
>   		LOG(RkISP1, Error)
>   			<< "Failed to start IPA " << camera->id();
>   		return ret;
>   	}
> +	cleaner.defer([&]() { data->ipa_->stop(); });
>   
>   	data->frame_ = 0;
>   
>   	ret = param_->streamOn();
>   	if (ret) {
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>   		LOG(RkISP1, Error)
>   			<< "Failed to start parameters " << camera->id();
>   		return ret;
>   	}
> +	cleaner.defer([&]() { param_->streamOff(); });
>   
>   	ret = stat_->streamOn();
>   	if (ret) {
> -		param_->streamOff();
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>   		LOG(RkISP1, Error)
>   			<< "Failed to start statistics " << camera->id();
>   		return ret;
>   	}
> +	cleaner.defer([&]() { stat_->streamOff(); });
>   
>   	if (data->mainPath_->isEnabled()) {
>   		ret = mainPath_.start();
> -		if (ret) {
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> +		if (ret)
>   			return ret;
> -		}
> +		cleaner.defer([&]() { mainPath_.stop(); });
>   	}
>   
>   	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
>   		ret = selfPath_.start();
> -		if (ret) {
> -			mainPath_.stop();
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> +		if (ret)
>   			return ret;
> -		}
>   	}
>   
>   	isp_->setFrameStartEnabled(true);
>   
>   	activeCamera_ = camera;
> -	return ret;
> +
> +	cleaner.clear();
> +	return 0;
>   }
>   
>   void PipelineHandlerRkISP1::stopDevice(Camera *camera)

Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Nicolas Dufresne via libcamera-devel Oct. 20, 2022, 5:46 a.m. UTC | #2 
On Tue, Oct 04, 2022 at 05:18:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Error handling in the PipelineHandlerRkISP1::start() function is
> cumbersome. Simplify it using the utils::Cleaner class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 +++++++++---------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 455ee2a0a711..33f3b0c5ab00 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -823,69 +823,60 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>  int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> +	utils::Cleaner cleaner;
>  	int ret;
>  
>  	/* Allocate buffers for internal pipeline usage. */
>  	ret = allocateBuffers(camera);
>  	if (ret)
>  		return ret;
> +	cleaner.defer([&]() { freeBuffers(camera); });
>  
>  	ret = data->ipa_->start();
>  	if (ret) {
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start IPA " << camera->id();
>  		return ret;
>  	}
> +	cleaner.defer([&]() { data->ipa_->stop(); });
>  
>  	data->frame_ = 0;
>  
>  	ret = param_->streamOn();
>  	if (ret) {
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start parameters " << camera->id();
>  		return ret;
>  	}
> +	cleaner.defer([&]() { param_->streamOff(); });
>  
>  	ret = stat_->streamOn();
>  	if (ret) {
> -		param_->streamOff();
> -		data->ipa_->stop();
> -		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start statistics " << camera->id();
>  		return ret;
>  	}
> +	cleaner.defer([&]() { stat_->streamOff(); });
>  
>  	if (data->mainPath_->isEnabled()) {
>  		ret = mainPath_.start();
> -		if (ret) {
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> +		if (ret)
>  			return ret;
> -		}
> +		cleaner.defer([&]() { mainPath_.stop(); });
>  	}
>  
>  	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
>  		ret = selfPath_.start();
> -		if (ret) {
> -			mainPath_.stop();
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	isp_->setFrameStartEnabled(true);
>  
>  	activeCamera_ = camera;
> -	return ret;
> +
> +	cleaner.clear();
> +	return 0;
>  }
>  
>  void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 455ee2a0a711..33f3b0c5ab00 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -823,69 +823,60 @@  int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
 int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
 {
 	RkISP1CameraData *data = cameraData(camera);
+	utils::Cleaner cleaner;
 	int ret;
 
 	/* Allocate buffers for internal pipeline usage. */
 	ret = allocateBuffers(camera);
 	if (ret)
 		return ret;
+	cleaner.defer([&]() { freeBuffers(camera); });
 
 	ret = data->ipa_->start();
 	if (ret) {
-		freeBuffers(camera);
 		LOG(RkISP1, Error)
 			<< "Failed to start IPA " << camera->id();
 		return ret;
 	}
+	cleaner.defer([&]() { data->ipa_->stop(); });
 
 	data->frame_ = 0;
 
 	ret = param_->streamOn();
 	if (ret) {
-		data->ipa_->stop();
-		freeBuffers(camera);
 		LOG(RkISP1, Error)
 			<< "Failed to start parameters " << camera->id();
 		return ret;
 	}
+	cleaner.defer([&]() { param_->streamOff(); });
 
 	ret = stat_->streamOn();
 	if (ret) {
-		param_->streamOff();
-		data->ipa_->stop();
-		freeBuffers(camera);
 		LOG(RkISP1, Error)
 			<< "Failed to start statistics " << camera->id();
 		return ret;
 	}
+	cleaner.defer([&]() { stat_->streamOff(); });
 
 	if (data->mainPath_->isEnabled()) {
 		ret = mainPath_.start();
-		if (ret) {
-			param_->streamOff();
-			stat_->streamOff();
-			data->ipa_->stop();
-			freeBuffers(camera);
+		if (ret)
 			return ret;
-		}
+		cleaner.defer([&]() { mainPath_.stop(); });
 	}
 
 	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
 		ret = selfPath_.start();
-		if (ret) {
-			mainPath_.stop();
-			param_->streamOff();
-			stat_->streamOff();
-			data->ipa_->stop();
-			freeBuffers(camera);
+		if (ret)
 			return ret;
-		}
 	}
 
 	isp_->setFrameStartEnabled(true);
 
 	activeCamera_ = camera;
-	return ret;
+
+	cleaner.clear();
+	return 0;
 }
 
 void PipelineHandlerRkISP1::stopDevice(Camera *camera)