[libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error value

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:
> libcamera::Request returned by Camera::requestComplete() is
> always assumed. Therefore, an error occurred in
> PipelineHandler::queueRequest() is ignored.
> This adds an error value, which is zero on success, to
> libcamera::Request() so that a Camera client can know the request
> error in completing request.

As the result is only valid when status() is Request::Complete, it
creates a risk for libcamera to set status() and result() to incoherent
values, and for applications to badly interpret them. I think it would
be better to instead extend Request::Status with a RequestError. We may
later need to distinguish between different types of errors, but until
we reach that point, extending the status() will be simpler.

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/request.h        |  2 ++
>  src/libcamera/pipeline_handler.cpp |  6 ++++--
>  src/libcamera/request.cpp          | 21 ++++++++++++++++-----
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 6e5aad5f..60e91d5f 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -52,6 +52,7 @@ public:
>  
>  	uint64_t cookie() const { return cookie_; }
>  	Status status() const { return status_; }
> +	int result() const { return result_; }
>  
>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>  
> @@ -73,6 +74,7 @@ private:
>  
>  	const uint64_t cookie_;
>  	Status status_;
> +	int result_;
>  	bool cancelled_;
>  };
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 05b807d6..a9a5523b 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)
>  	data->queuedRequests_.push_back(request);
>  
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret)
> -		data->queuedRequests_.remove(request);
> +	if (ret) {
> +		request->result_ = ret;
> +		completeRequest(request);
> +	}
>  }
>  
>  /**
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 0071667e..8106437e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)
>   *
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -	: camera_(camera), cookie_(cookie), status_(RequestPending),
> +	: camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),
>  	  cancelled_(false)
>  {
>  	/**
> @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   * \fn Request::status()
>   * \brief Retrieve the request completion status
>   *
> - * The request status indicates whether the request has completed successfully
> - * or with an error. When requests are created and before they complete the
> - * request status is set to RequestPending, and is updated at completion time
> - * to RequestComplete. If a request is cancelled at capture stop before it has
> + * The request status indicates whether the request has pended, completed or
> + * cancelled.. When requests are created and before they complete the request
> + * status is set to RequestPending, and is updated at completion time to
> + * RequestComplete. If a request is cancelled at capture stop before it has
>   * completed, its status is set to RequestCancelled.
>   *
>   * \return The request completion status
>   */
>  
> +/**
> + * \fn Request::result()
> + * \brief Retrieve the error value of the request .
> + *
> + * The request error indicates whether the request has completed successfully or
> + * with an error. It is a negative value if an error happens in queueing and
> + * processing the request, or 0 on success.
> + *
> + * \return The request error value.
> + */
> +
>  /**
>   * \fn Request::hasPendingBuffers()
>   * \brief Check if a request has buffers yet to be completed

-- 
Regards,

Laurent Pinchart