[libcamera-devel] [PATCH] cam: capture: Stop stream when queueRequest fails

[Helen Koike]

Hi Kieran,

On 6/25/19 4:51 PM, Kieran Bingham wrote:
> Hi Helen,
> 
> Thank you for the patch, and welcome to libcamera !
> 
> On 25/06/2019 17:23, Helen Koike wrote:
>> queueRequest is called after starting the stream.
> 
> Very minor, We normally put '()' to denote functions so it would be good
> to put queueRequest()

Good to know.

> 
> I can add those while applying.

Thanks!

> 
> 
>> If it fails, the stream should be stopped, otherwise it can get a
>> "Device or resource busy" error, due to VIDIOC_REQBUFS ioctls being
>> called after VIDIOC_STREAMON without VIDIOC_STREAMOFF in-between.
> 
> Sounds like a good find. I hope this means that libcamera is starting to
> be useful already.

It is :)

> 
> Out of curiosity, what platform are you running this on? RK3399 perhaps?

Yes, rk3399.

Regards,
Helen

> 
> 
>> Signed-off-by: Helen Koike <helen.koike at collabora.com>
>> ---
>>  src/cam/capture.cpp | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
>> index e455612..6b842d7 100644
>> --- a/src/cam/capture.cpp
>> +++ b/src/cam/capture.cpp
>> @@ -117,6 +117,7 @@ int Capture::capture(EventLoop *loop)
>>  		ret = camera_->queueRequest(request);
>>  		if (ret < 0) {
>>  			std::cerr << "Can't queue request" << std::endl;
>> +			camera_->stop();
> 
> This looks good to me,
> 
> It seems clear that this is an unbalanced start()/stop() code path in
> the cam utility. I started to wonder while reviewing this if we should
> have a call somewhere that stops the camera on an error, but I think
> that goes beyond the scope of this patch and handling errors with magic
> will be some extra policy :-)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
>>  			return ret;
>>  		}
>>  	}
>>
>