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

[Jacopo Mondi]

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 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 :)

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