[libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate freeBuffers() error

[Jacopo Mondi]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190416/14adf464/attachment.sig>