| Message ID | 20190415231859.9747-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote: > The error return code of PipelineHandler::freeBuffers() was not > propagate up to applications by the Camera class. Fix this by returning > the incremental error accumulated by multiple calls (one per Stream) > to freeBuffers(). > > Do not return the error code of the call to freeBuffers() in the > allocateBuffers() method error path not to overwrite the original error > code returned from the allocation method. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index bdf14b31d8ee..7f2dc904df16 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -671,6 +671,8 @@ int Camera::allocateBuffers() > */ > int Camera::freeBuffers() > { > + int ret = 0; > + > if (!stateIs(CameraPrepared)) > return -EACCES; > > @@ -683,12 +685,12 @@ int Camera::freeBuffers() > * by the V4L2 device that has allocated them. > */ > stream->bufferPool().destroyBuffers(); > - pipe_->freeBuffers(this, stream); > + ret |= pipe_->freeBuffers(this, stream); Or:ing in errors here seems very strange as we are dealing with the standard error codes (EINVAL, EBUSY, etc). Would it not be saner to store and return the (potential) first error while still running freeBuffers() for all streams before returning? How about something like this int ret = 0; for (Stream *stream : activeStreams_) { int status = pipe_->freeBuffers(this, stream); if (!ret) ret = status; } return ret; > } > > state_ = CameraConfigured; > > - return 0; > + return ret; > } > > /** > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Tue, Apr 16, 2019 at 01:43:28AM +0200, Niklas Söderlund wrote: > On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote: > > The error return code of PipelineHandler::freeBuffers() was not > > propagate up to applications by the Camera class. Fix this by returning > > the incremental error accumulated by multiple calls (one per Stream) > > to freeBuffers(). > > > > Do not return the error code of the call to freeBuffers() in the > > allocateBuffers() method error path not to overwrite the original error > > code returned from the allocation method. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/camera.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index bdf14b31d8ee..7f2dc904df16 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -671,6 +671,8 @@ int Camera::allocateBuffers() > > */ > > int Camera::freeBuffers() > > { > > + int ret = 0; > > + > > if (!stateIs(CameraPrepared)) > > return -EACCES; > > > > @@ -683,12 +685,12 @@ int Camera::freeBuffers() > > * by the V4L2 device that has allocated them. > > */ > > stream->bufferPool().destroyBuffers(); > > - pipe_->freeBuffers(this, stream); > > + ret |= pipe_->freeBuffers(this, stream); > > Or:ing in errors here seems very strange as we are dealing with the > standard error codes (EINVAL, EBUSY, etc). Would it not be saner to > store and return the (potential) first error while still running > freeBuffers() for all streams before returning? > > How about something like this > > int ret = 0; > > for (Stream *stream : activeStreams_) { > int status = pipe_->freeBuffers(this, stream); > > if (!ret) > ret = status; > } > > return ret; Or maybe merging this on top of the patch that refactors buffer allocation ? The loop will then be gone. > > } > > > > state_ = CameraConfigured; > > > > - return 0; > > + return ret; > > } > > > > /**
Hi, On Tue, Apr 16, 2019 at 01:55:27PM +0300, Laurent Pinchart wrote: > Hello, > > On Tue, Apr 16, 2019 at 01:43:28AM +0200, Niklas Söderlund wrote: > > On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote: > > > The error return code of PipelineHandler::freeBuffers() was not > > > propagate up to applications by the Camera class. Fix this by returning > > > the incremental error accumulated by multiple calls (one per Stream) > > > to freeBuffers(). > > > > > > Do not return the error code of the call to freeBuffers() in the > > > allocateBuffers() method error path not to overwrite the original error > > > code returned from the allocation method. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/camera.cpp | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index bdf14b31d8ee..7f2dc904df16 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -671,6 +671,8 @@ int Camera::allocateBuffers() > > > */ > > > int Camera::freeBuffers() > > > { > > > + int ret = 0; > > > + > > > if (!stateIs(CameraPrepared)) > > > return -EACCES; > > > > > > @@ -683,12 +685,12 @@ int Camera::freeBuffers() > > > * by the V4L2 device that has allocated them. > > > */ > > > stream->bufferPool().destroyBuffers(); > > > - pipe_->freeBuffers(this, stream); > > > + ret |= pipe_->freeBuffers(this, stream); > > > > Or:ing in errors here seems very strange as we are dealing with the > > standard error codes (EINVAL, EBUSY, etc). Would it not be saner to > > store and return the (potential) first error while still running > > freeBuffers() for all streams before returning? > > > > How about something like this > > > > int ret = 0; > > > > for (Stream *stream : activeStreams_) { > > int status = pipe_->freeBuffers(this, stream); > > > > if (!ret) > > ret = status; > > } > > > > return ret; > > Or maybe merging this on top of the patch that refactors buffer > allocation ? The loop will then be gone. > Ah! so you would like to have it like it was in v4? It made more sense to me as well, I think I've been asked to split it, so I did. > > > } > > > > > > state_ = CameraConfigured; > > > > > > - return 0; > > > + return ret; > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index bdf14b31d8ee..7f2dc904df16 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -671,6 +671,8 @@ int Camera::allocateBuffers() */ int Camera::freeBuffers() { + int ret = 0; + if (!stateIs(CameraPrepared)) return -EACCES; @@ -683,12 +685,12 @@ int Camera::freeBuffers() * by the V4L2 device that has allocated them. */ stream->bufferPool().destroyBuffers(); - pipe_->freeBuffers(this, stream); + ret |= pipe_->freeBuffers(this, stream); } state_ = CameraConfigured; - return 0; + return ret; } /**
The error return code of PipelineHandler::freeBuffers() was not propagate up to applications by the Camera class. Fix this by returning the incremental error accumulated by multiple calls (one per Stream) to freeBuffers(). Do not return the error code of the call to freeBuffers() in the allocateBuffers() method error path not to overwrite the original error code returned from the allocation method. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)