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

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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.

Indeed, this sounds better.

> 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.

A Request::Status argument could cause questions about possible
Request::Status::Pending value.  Which could be avoided by using
something like `bool cancel' argument instead but I think it's better to
stop here and leave things as they are; other than that I also 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