[libcamera-devel] [PATCH v2 3/8] libcamera: v4l2_device: streamOff() when releasing buffers

[Laurent Pinchart]

On Wed, Feb 13, 2019 at 04:30:36PM +0000, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 13/02/2019 15:41, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Feb 13, 2019 at 03:10:22PM +0000, Kieran Bingham wrote:
> >> We must ensure that the stream is disabled before releasing buffers. It will
> >> not hurt to call streamOff() even if it is already off before releasing any
> >> buffers back to the device.
> > 
> > I'm not sure about this one. Doesn't it indicate a problem in the caller
> > instead ?
> 
> Perhaps. It means (in the cases that I've hit) that the error paths have
> not called streamOff().
> 
> Is it more preferable to leave buffers 'un-released'?

I think that would be better than trying to hide the issue, yes.

> Perhaps would you prefer to see releaseBuffers return an error if it
> fails?

Sounds good to me, with an error message logged.

> As this is likely to be on cleanup paths, and destruction - what
> would you expect the application to do in the event of failure to
> releaseBuffers? (I guess - fix their code...)

Yes, fix their code :-) Buffers can also be released at runtime when
switching resolutions, in which case proper cleanup is mandatory, and
errors in the code flow should be logged instead of hidden.

> Either way - I'll just drop this patch for now.
> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/v4l2_device.cpp | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> index 23c0da295905..83073c28b817 100644
> >> --- a/src/libcamera/v4l2_device.cpp
> >> +++ b/src/libcamera/v4l2_device.cpp
> >> @@ -642,6 +642,8 @@ int V4L2Device::releaseBuffers()
> >>  {
> >>  	LOG(V4L2, Debug) << "Releasing bufferPool";
> >>  
> >> +	streamOff();
> >> +
> >>  	requestBuffers(0);
> >>  	bufferPool_ = nullptr;
> >>  

-- 
Regards,

Laurent Pinchart