[libcamera-devel] [PATCH v2 2/8] libcamera: request: Add Request::cancel()

[Kieran Bingham]

Hi Jacopo,

On 13/05/2021 10:22, Jacopo Mondi wrote:
> Add a cancel() function to the Request class that allows to forcefully
> complete the request and its associated buffers in error state.
> 
> Only pending requests can be forcefully cancelled. Enforce that
> by asserting the request state to be RequestPending.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/request.h |  1 +
>  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 4cf5ff3f7d3b..5596901ddd8e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -65,6 +65,7 @@ private:
>  	friend class PipelineHandler;
>  
>  	void complete();
> +	void cancel();
>  
>  	bool completeBuffer(FrameBuffer *buffer);
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ce2dd7b17f10..fc5e25199112 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -292,6 +292,36 @@ void Request::complete()
>  	LIBCAMERA_TRACEPOINT(request_complete, this);
>  }
>  
> +/**
> + * \brief Cancel a queued request
> + *
> + * Mark the request and its associated buffers as cancelled and complete it.
> + *
> + * Set the status of each buffer in the request to the frame cancelled state and
> + * remove them from the pending buffer queue before completing the request with
> + * error.
> + */
> +void Request::cancel()
> +{
> +	LIBCAMERA_TRACEPOINT(request_cancel, this);

Have you checked that this is sufficient?

Doesn't this require an addition in
   include/libcamera/internal/tracepoints/request.tp


> +
> +	ASSERT(status_ == RequestPending);
> +
> +	/*
> +	 * We can't simply loop and call completeBuffer() as erase() invalidates
> +	 * pointers and iterators, so we have to manually cancel the buffer and
> +	 * erase it from the pending buffers list.
> +	 */
> +	for (auto buffer = pending_.begin(); buffer != pending_.end();) {
> +		(*buffer)->cancel();

Do buffers need to notify a completion event if they're cancelled? I
don't think they do - but I'm curious if there is a use case to be
notified in a buffer event handler of a buffer being cancelled...



> +		(*buffer)->setRequest(nullptr);

Even if the buffer is cancelled, it's still associated with this request
isn't it?

Does it need to be set to null?


> +		buffer = pending_.erase(buffer);
> +	}
> +
> +	cancelled_ = true;
> +	complete();
> +}
> +
>  /**
>   * \brief Complete a buffer for the request
>   * \param[in] buffer The buffer that has completed
> 

-- 
Regards
--
Kieran