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

[Xavier Roumegue (OSS)]

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 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 ?

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