[libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add RequestError to Status

[Jacopo Mondi]

Hi Hiro,

On Mon, Mar 29, 2021 at 06:15:51PM +0900, Hirokazu Honda wrote:
> This adds a new libcamera::Request::Status, RequestError, which
> represents a request isn't successfully processed due to some
> error.
>

It seems to me that RequestCancelled is now only set if the frame
metadata are cancelled by the IPA and is signaled by the pipeline
handlers by calling completeBuffer() and have Request detect such an
error condition.

bool Request::completeBuffer(FrameBuffer *buffer)
{
	LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);

	int ret = pending_.erase(buffer);
	ASSERT(ret == 1);

	buffer->request_ = nullptr;

	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
		cancelled_ = true;

	return !hasPendingBuffers();
}

The cancelled request state is then propagate to application at
request completion

void Request::complete()
{
	ASSERT(status_ == RequestPending);
	ASSERT(!hasPendingBuffers());

	status_ = cancelled_ ? RequestCancelled : RequestComplete;

	LOG(Request, Debug)
		<< "Request has completed - cookie: " << cookie_
		<< (cancelled_ ? " [Cancelled]" : "");

	LIBCAMERA_TRACEPOINT(request_complete, this);
}

Wouldn't it be better to keep using RequestComplete and add an error_
field like you're doing here to be inspected by Request::complete()
that could transport different errors ? In example

        enum RequestError {
                MetadataCancelled,
                BufferUnderrun,
                ...
        };

And change completeBuffers() to use the new error field as well as
PipelineHandler::queueRequest() and change Request::complete() to
inspect the new field so that you can drop cancelled_ ?

In this way a failed Request will always have status ==
RequestCancelled (which can be changed to RequestFailed if we like it
more) and the exact failure reason can be deduced inspecting
Request::error() ?

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/request.h        |  2 ++
>  src/libcamera/pipeline_handler.cpp |  5 ++++-
>  src/libcamera/request.cpp          | 17 +++++++++++------
>  3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 6e5aad5f..598fcf86 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -30,6 +30,7 @@ public:
>  		RequestPending,
>  		RequestComplete,
>  		RequestCancelled,
> +		RequestError,
>  	};
>
>  	enum ReuseFlag {
> @@ -73,6 +74,7 @@ private:
>
>  	const uint64_t cookie_;
>  	Status status_;
> +	bool error_;
>  	bool cancelled_;
>  };
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index eba00ed3..66326ce0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -381,8 +381,11 @@ void PipelineHandler::queueRequest(Request *request)
>  	data->queuedRequests_.push_back(request);
>
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret)
> +	if (ret) {
> +		request->error_ = true;
>  		data->queuedRequests_.remove(request);
> +		completeRequest(request);
> +	}
>  }
>
>  /**
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 0071667e..176f59dd 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -36,6 +36,8 @@ LOG_DEFINE_CATEGORY(Request)
>   * The request has completed
>   * \var Request::RequestCancelled
>   * The request has been cancelled due to capture stop
> + * \var Request::RequestError
> + * The request is not completed successfully due to an error.
>   */
>
>  /**
> @@ -73,7 +75,7 @@ LOG_DEFINE_CATEGORY(Request)
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
>  	: camera_(camera), cookie_(cookie), status_(RequestPending),
> -	  cancelled_(false)
> +	  error_(false), cancelled_(false)
>  {
>  	/**
>  	 * \todo Should the Camera expose a validator instance, to avoid
> @@ -126,6 +128,7 @@ void Request::reuse(ReuseFlag flags)
>  	}
>
>  	status_ = RequestPending;
> +	error_ = false;
>  	cancelled_ = false;
>
>  	controls_->clear();
> @@ -256,20 +259,22 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>  /**
>   * \brief Complete a queued request
>   *
> - * Mark the request as complete by updating its status to RequestComplete,
> - * unless buffers have been cancelled in which case the status is set to
> - * RequestCancelled.
> + * Mark the request as complete by updating its status to RequestError on error,
> + * RequestCancelled if buffers have been cancelled, or otherwise RequestComplete.
>   */
>  void Request::complete()
>  {
>  	ASSERT(status_ == RequestPending);
>  	ASSERT(!hasPendingBuffers());
>
> -	status_ = cancelled_ ? RequestCancelled : RequestComplete;
> +	if (error_)
> +		status_ = RequestError;
> +	else
> +		status_ = cancelled_ ? RequestCancelled : RequestComplete;
>
>  	LOG(Request, Debug)
>  		<< "Request has completed - cookie: " << cookie_
> -		<< (cancelled_ ? " [Cancelled]" : "");
> +		<< (error_ ? " [Error]" : (cancelled_ ? " [Cancelled]" : ""));
>
>  	LIBCAMERA_TRACEPOINT(request_complete, this);
>  }
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel