[libcamera-devel] [PATCH v3 22/22] libcamera: pipeline: rkisp1: Use cleanup error paths for start()

[Niklas Söderlund]

Hi Jacopo,

On 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 03:42:07AM +0200, Niklas Söderlund wrote:
> > Use a lambda function to make the error paths a bit cleaner in start(),
> > no functional change.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------
> >  1 file changed, 51 insertions(+), 47 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index c6c2e3aa3dc6d0dc..34196af3057b14bb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -827,58 +827,62 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >  	RkISP1CameraData *data = cameraData(camera);
> >  	int ret;
> >
> > -	/* Allocate buffers for internal pipeline usage. */
> > -	ret = allocateBuffers(camera);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = data->ipa_->start();
> > -	if (ret) {
> > -		freeBuffers(camera);
> > -		LOG(RkISP1, Error)
> > -			<< "Failed to start IPA " << camera->id();
> > -		return ret;
> > -	}
> > -
> > -	data->frame_ = 0;
> > -
> > -	ret = param_->streamOn();
> > -	if (ret) {
> > -		data->ipa_->stop();
> > -		freeBuffers(camera);
> > -		LOG(RkISP1, Error)
> > -			<< "Failed to start parameters " << camera->id();
> > -		return ret;
> > -	}
> > -
> > -	ret = stat_->streamOn();
> > -	if (ret) {
> > -		param_->streamOff();
> > -		data->ipa_->stop();
> > -		freeBuffers(camera);
> > -		LOG(RkISP1, Error)
> > -			<< "Failed to start statistics " << camera->id();
> > -		return ret;
> > -	}
> > -
> > -	ret = mainPath_.start();
> > -	if (ret) {
> > -		param_->streamOff();
> > -		stat_->streamOff();
> > -		data->ipa_->stop();
> > -		freeBuffers(camera);
> > -		return ret;
> > -	}
> > -
> > -	ret = selfPath_.start();
> > -	if (ret) {
> > +	auto startDevices = [this](Camera *camera, RkISP1CameraData *data) {
> 
> Why a lamda function for a function that is only called for a single place as a
> regulr function ?

It was suggested in review of an earlier version to make the code more 
readable. But as the cover letter suggests this 22/22 should be thought 
of as a RFC. I found it rather neat to make the code more readable.

> 
> Also, shouldn't re-working the existing code to use labels be better ?

Maybe, it's such a larger function so I thought it neater to crate a 
separate "block" for the code needing error handling. If this was C I 
would have created a helper function just to get it out but creating a 
helper class function for this seems a bit much so a lambda was 
somewhere in the middle.

Maybe I will break this out of this series so people don't spend all 
review energy bikeshedding over this one ;-P

> 
> > +		int ret;
> > +
> > +		/* Allocate buffers for internal pipeline usage. */
> > +		ret = allocateBuffers(camera);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = data->ipa_->start();
> > +		if (ret) {
> > +			LOG(RkISP1, Error)
> > +				<< "Failed to start IPA " << camera->id();
> > +			goto err_allocate;
> > +		}
> > +
> > +		data->frame_ = 0;
> > +
> > +		ret = param_->streamOn();
> > +		if (ret) {
> > +			LOG(RkISP1, Error)
> > +				<< "Failed to start parameters " << camera->id();
> > +			goto err_ipa;
> > +		}
> > +
> > +		ret = stat_->streamOn();
> > +		if (ret) {
> > +			LOG(RkISP1, Error)
> > +				<< "Failed to start statistics " << camera->id();
> > +			goto err_param;
> > +		}
> > +
> > +		ret = mainPath_.start();
> > +		if (ret)
> > +			goto err_stat;
> > +
> > +		ret = selfPath_.start();
> > +		if (ret)
> > +			goto err_main;
> > +
> > +		return 0;
> > +	err_main:
> >  		mainPath_.stop();
> > -		param_->streamOff();
> > +	err_stat:
> >  		stat_->streamOff();
> > +	err_param:
> > +		param_->streamOff();
> > +	err_ipa:
> >  		data->ipa_->stop();
> > +	err_allocate:
> >  		freeBuffers(camera);
> >  		return ret;
> > -	}
> > +	};
> > +
> > +	ret = startDevices(camera, data);
> > +	if (ret)
> > +		return ret;
> >
> >  	activeCamera_ = camera;
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund