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

[Niklas Söderlund]

Hi Jacopo,

On 2020-09-28 11:09:03 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 06:57:31PM +0200, Niklas Söderlund wrote:
> > 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
> 
> Just for sake of discussion:

:-)

> 
> It's mostly for the fact that it's called from a single point that
> means it's not necesasry to use lambda in my opinion (also, why do you
> need to capture [this] ?)

I need [this] to allow the labmda to access the class member variables.

> 
> I would see a lambda fit if you were to iterate on an array of video
> devices and perform the same action on all of them like (not compiled
> or tested):
> 
>         std::array<V4L2VideoDevice *, 4> videoDevices{
>                 param_, stat_, main_, self_
>         };
> 
>         /* Allocate buffers for internal pipeline usage. */
>         ret = allocateBuffers(camera);
>         if (ret)
>                 return ret;
> 
>         int ret;
>         std::for_each(std::begin(videoDevices),
>                       std::end(videoDevices),
>                       [&ret](const V4L2VideoDevice &vdev) {
>                               ret = vdev.streamOn();
>                               if (ret)
>                                       goto stopDevices;
>                       }
>         }
> 
>         return 0;
> 
>         stopDevices:
>                 freeBuffers(camera);
>                 std::for_each(std::begin(videoDevices),
>                               std::end(videoDevices),
>                               [](const V4L2VideoDevice &vdev) {
>                                       vdev.streamOff();
>                               }
>                 }
> 
>                 return ret;
> 
> But you have both V4L2VideoDevice to start with streamOn() and
> RkISP1Path to start with start() so I don't see it very neat unless
> you create two different arrays or wrap the stat_ and param_
> V4L2VideoDevice * in a RkISP1Path.
> 
> Just adding labels to the existing seems easier :)

Well one could do that but then we need to declare all variables before 
the first goto. But as stated elsewhere this was a fun RFC but I will 
drop it.

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

-- 
Regards,
Niklas Söderlund