| Message ID | 20200925014207.1455796-23-niklas.soderlund@ragnatech.se |
|---|---|
| State | Rejected |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? Also, shouldn't re-working the existing code to use labels be better ? > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > 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@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Mon, Sep 28, 2020 at 11:50:12PM +0200, Niklas Söderlund wrote: > On 2020-09-28 11:09:03 +0200, Jacopo Mondi wrote: > > On Fri, Sep 25, 2020 at 06:57:31PM +0200, Niklas Söderlund wrote: > > > On 2020-09-25 17:36:00 +0200, Jacopo Mondi wrote: > > > > 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@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. For the record, I've suggested a cleanup mechanism, not a start mechanism :-) > > > > 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. You could still create a separate private function in the class. > > > 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. In its current form I agree with Jacopo, I'm not very fond of it. As a cleanup handler, however, I think it would be nice. Making the individual cleanup functions idempotent (when they're not already) won't have any noticeable impact on performances are they're not call in hot paths, and would make cleanup paths simpler in all pipeline handlers. > > > > > + 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; > > > > >
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) { + 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;
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@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 98 ++++++++++++------------ 1 file changed, 51 insertions(+), 47 deletions(-)