[libcamera-devel] [PATCH v2 3/3] Regard a request error in the request completion

[Kieran Bingham]

Hi Hiro,

On 29/03/2021 10:15, Hirokazu Honda wrote:
> libcamera::Request contains an error value. Every
> libcamera::Camera client should regards the error in a
> request completion.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  Documentation/guides/application-developer.rst | 2 ++
>  src/cam/capture.cpp                            | 5 +++++
>  src/gstreamer/gstlibcamerasrc.cpp              | 6 +++++-
>  src/qcam/main_window.cpp                       | 4 ++++
>  src/v4l2/v4l2_camera.cpp                       | 5 +++++
>  5 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index e3430f03..e8a1a7b4 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -392,6 +392,8 @@ the `Request::Status`_ class enum documentation.
>  
>     if (request->status() == Request::RequestCancelled)
>        return;
> +   if (request->status() == Request::RequestError)
> +      // handle error.

Even if it's just documentation, we should wrap that // handle error. in
{ } as it leaves the if () statement applying to whatever would happen
next otherwise...



>  If the ``Request`` has completed successfully, applications can access the
>  completed buffers using the ``Request::buffers()`` function, which returns a map
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 7b55fc67..3e9edf80 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -167,6 +167,11 @@ void Capture::requestComplete(Request *request)
>  {
>  	if (request->status() == Request::RequestCancelled)
>  		return;
> +	/* TODO: Handle an error correctly */
> +	if (request->status() == Request::RequestError) {
> +		std::cout << "Failed to capture request: " << request->cookie();
> +		return;

I don't think we can let this return early here. It would cause us to
drop errorred frames, and then run out of active requests.

If a Request status is RequestError, then we need to decide if the frame
is to be dropped or displayed regardless. (which is an application
choice, rather than a libcamera choice). That's fine as we're here in cam..

An application which wants to display only 'good' frames would likely
choose to not display this frame, but it would then need to make sure
that a new request would be queued still.

For cam, I think we *should* track errored frames, as they are important
in testing.

So this should certainly call into Capture::processRequest(), and likely
print a notice saying that this frame was an error. And still save it
accordingly...

Although, I'm not sure what RequestError means yet.
Does it mean there was an error and the Request wasn't processed
completely, or that it wasn't processed at all.

Perhaps the use case for a buffer not completely being processed is
already handled by the buffer status...



> +	}
>  
>  	/*
>  	 * Defer processing of the completed request to the event loop, to avoid
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 87246b40..1ecb9883 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -163,10 +163,14 @@ GstLibcameraSrcState::requestCompleted(Request *request)
>  
>  	g_return_if_fail(wrap->request_.get() == request);
>  
> -	if ((request->status() == Request::RequestCancelled)) {
> +	if (request->status() == Request::RequestCancelled) {
>  		GST_DEBUG_OBJECT(src_, "Request was cancelled");
>  		return;
>  	}
> +	if (request->status() == Request::RequestError) {
> +		GST_ERROR_OBJECT(src_, "Request doesn't complete successfully");

s/doesn't/didn't/

I haven't looked into how gstlibcamerasrc uses it's requests yet, but if
this leaks a request and causes the pipeline to stall that would be bad too.


> +		return;
> +	}
>  
>  	GstBuffer *buffer;
>  	for (GstPad *srcpad : srcpads_) {
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 39d034de..1288bcd5 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -694,6 +694,10 @@ void MainWindow::requestComplete(Request *request)
>  {
>  	if (request->status() == Request::RequestCancelled)
>  		return;
> +	if (request->status() == Request::RequestError) {
> +		qCritical() << "Request doesn't complete successfully";

s/doesn't/didn't/

And the same concerns of course.

> +		return;
> +	}
>  
>  	/*
>  	 * We're running in the libcamera thread context, expensive operations
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 97825c71..d6b1d755 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -84,6 +84,11 @@ void V4L2Camera::requestComplete(Request *request)
>  {
>  	if (request->status() == Request::RequestCancelled)
>  		return;
> +	if (request->status() == Request::RequestError) {
> +		LOG(V4L2Compat, Error)
> +			<< "Request doesn't complete successfully";

And same ;-)

> +		return;
> +	}
>  
>  	/* We only have one stream at the moment. */
>  	bufferLock_.lock();
> 

-- 
Regards
--
Kieran