[PATCH v5 1/3] libcamera: pipeline_handler: Provide cancelRequest

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote:
> Let's extract the two occurrences of canceling a request to a common
> helper.  This is especially useful for the followup patch, which needs
> to cancel a request from outside.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 0d3808036..fb28a18d0 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -60,6 +60,7 @@ public:
>  
>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>  	void completeRequest(Request *request);
> +	void cancelRequest(Request *request);
>  
>  	std::string configurationFile(const std::string &subdir,
>  				      const std::string &name) const;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e59404691..c9cb11f0f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>  	while (!waitingRequests_.empty()) {
>  		Request *request = waitingRequests_.front();
>  		waitingRequests_.pop();
> -
> -		request->_d()->cancel();
> -		completeRequest(request);
> +		cancelRequest(request);
>  	}
>  
>  	/* Make sure no requests are pending. */
> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>  	}
>  
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret) {
> -		request->_d()->cancel();
> -		completeRequest(request);
> -	}
> +	if (ret)
> +		cancelRequest(request);
>  }
>  
>  /**
> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>  	}
>  }
>  
> +/**
> + * \brief Cancel request and signal its completion
> + * \param[in] request The request to cancel
> + *
> + * This function cancels the request in addition to its completion. The same

"in additiona to its completion" confuses me. I would simply write

 * This function cancels and completes the request. The same rules as for
 * completeRequest() apply.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Although, I wonder if we could instead add a Request::Status argument to
completeRequest(), default it to Request::Status::RequestComplete, and
avoid adding a new function. I don't mind much either way.

> + * rules as for completeRequest() apply.
> + */
> +void PipelineHandler::cancelRequest(Request *request)
> +{
> +	request->_d()->cancel();
> +	completeRequest(request);
> +}
> +
>  /**
>   * \brief Retrieve the absolute path to a platform configuration file
>   * \param[in] subdir The pipeline handler specific subdirectory name

-- 
Regards,

Laurent Pinchart